From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750913AbdA0Sey (ORCPT ); Fri, 27 Jan 2017 13:34:54 -0500 Received: from mx2.suse.de ([195.135.220.15]:41041 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750846AbdA0SeR (ORCPT ); Fri, 27 Jan 2017 13:34:17 -0500 Date: Fri, 27 Jan 2017 19:19:30 +0100 From: Borislav Petkov To: Rabin Vincent Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Petr Mladek , Sergey Senozhatsky Subject: Re: [PATCH] printk: fix printk.devkmsg sysctl Message-ID: <20170127181930.sshkgcocttbwu7k4@pd.tnic> References: <1485522706-18852-1-git-send-email-rabin.vincent@axis.com> <20170127150141.5w33ardlqab6rekz@pd.tnic> <20170127154230.GB3799@axis.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170127154230.GB3799@axis.com> User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org + printk folk. On Fri, Jan 27, 2017 at 04:42:30PM +0100, Rabin Vincent wrote: > proc_dostring() eats the '\n' and stops Not a problem, see diff below. Please do it this way instead because after a month no one will remember what this complex conditional means > + if (err < 0 || (err != *lenp && err + 1 != *lenp) || > + err != strlen(devkmsg_log_str)) { and why we did it this way. Also, having the different parts of the conditional explained with a comment makes it much more obvious. Also, > Before this patch: > > # cat /proc/sys/kernel/printk_devkmsg > ratelimit > # echo off > /proc/sys/kernel/printk_devkmsg > # sysctl -w kernel.printk_devkmsg=off > sysctl: short write > # echo -n off > /proc/sys/kernel/printk_devkmsg > -sh: echo: write error: Invalid argument > # echo -n offX > /proc/sys/kernel/printk_devkmsg > # > # printf "off\nX" >/proc/sys/kernel/printk_devkmsg > -sh: printf: write error: Invalid argument > > After this patch: > > # cat /proc/sys/kernel/printk_devkmsg > ratelimit > # echo off > /proc/sys/kernel/printk_devkmsg > # sysctl -w kernel.printk_devkmsg=off > kernel.printk_devkmsg = off > # echo -n off > /proc/sys/kernel/printk_devkmsg > # echo -n offX > /proc/sys/kernel/printk_devkmsg > -sh: echo: write error: Invalid argument > # printf "off\nX" >/proc/sys/kernel/printk_devkmsg > -sh: printf: write error: Invalid argument you can leave all those printk examples in the commit message but you should also explain why we're doing what we're doing. To allow this and that input and disallow others and why we need to "fish out" newline before proc_dostring(), yadda yadda. Thanks. --- diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 8b2696420abb..2a1f7c8efb16 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -156,14 +156,19 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, { char old_str[DEVKMSG_STR_MAX_SIZE]; unsigned int old; + bool newline = false; int err; if (write) { + char __user *b = buffer; + if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) return -EINVAL; old = devkmsg_log; strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE); + + newline = b[*lenp - 1] == '\n'; } err = proc_dostring(table, write, buffer, lenp, ppos); @@ -173,21 +178,27 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, if (write) { err = __control_devkmsg(devkmsg_log_str); - /* - * Do not accept an unknown string OR a known string with - * trailing crap... - */ - if (err < 0 || (err + 1 != *lenp)) { + /* Do not accept an unknown string... */ + if (err < 0) + goto restore; - /* ... and restore old setting. */ - devkmsg_log = old; - strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE); + /* ... known string without trailing '\n' is fine ... */ + if (err == *lenp) + return 0; - return -EINVAL; - } + /* ... so is known string with a trailing '\n'.*/ + if (err + 1 != *lenp || !newline) + goto restore; } return 0; + +restore: + /* Restore old setting. */ + devkmsg_log = old; + strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE); + + return -EINVAL; } /* --- -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --