From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751219AbdAaOoD (ORCPT ); Tue, 31 Jan 2017 09:44:03 -0500 Received: from mx2.suse.de ([195.135.220.15]:36503 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750894AbdAaOnu (ORCPT ); Tue, 31 Jan 2017 09:43:50 -0500 Date: Tue, 31 Jan 2017 14:55:01 +0100 From: Petr Mladek To: Borislav Petkov Cc: Rabin Vincent , akpm@linux-foundation.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH] printk: fix printk.devkmsg sysctl Message-ID: <20170131135501.GT20462@pathway.suse.cz> References: <1485522706-18852-1-git-send-email-rabin.vincent@axis.com> <20170127150141.5w33ardlqab6rekz@pd.tnic> <20170127154230.GB3799@axis.com> <20170127181930.sshkgcocttbwu7k4@pd.tnic> <20170130133803.GH6620@pathway.suse.cz> <20170130170318.gvcxx7ouy54rwy2i@pd.tnic> <20170130223911.i7ja5ywyhaaopsgu@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170130223911.i7ja5ywyhaaopsgu@pd.tnic> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon 2017-01-30 23:39:12, Borislav Petkov wrote: > On Mon, Jan 30, 2017 at 06:03:18PM +0100, Borislav Petkov wrote: > > IOW, I'd like the user to know what she does and mean it. No sloppy > > inputs. > > Ok, here's what I wanna do. I decided to do my own parsing on the write > path since proc_dostring() does not really allow me to look at the input > string. Here's what I have so far, I'll do some more testing tomorrow. > > I know, the diff is practically unreadable so let me paste the whole > function here. > > So this way I can really control which input is accepted and which not > without jumping through hoops and doing silly games with the length. > > And of course I'm not saving/restoring strings like a crazy person. This solution is cleaner. I am only afraid that we might miss some subtle sysctl specialities. For example, these functions seems to have some special handling of empty values: static int __do_proc_dointvec(void *tbl_data, struct ctl_table *table, [...] { [...] if (!tbl_data || !table->maxlen || !*lenp || (*ppos && !write)) { *lenp = 0; return 0; } And more importantly. There seems to be some strict write mode, see SYSCTL_WRITES_STRICT, SYSCTL_WRITES_WARN. Also we might need to shift *ppos by the length of written string. The original code got all these things for free from proc_dostring(). > int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, > void __user *buffer, size_t *lenp, loff_t *ppos) > { > char tmp_str[DEVKMSG_STR_MAX_SIZE]; [...] > len = min_t(int, (int)*lenp, DEVKMSG_STR_MAX_SIZE); [...] > err = copy_from_user(tmp_str, buffer, len); > if (err) > return -EFAULT; The code still reads only 10 bytes and could get cheated ;-) For example, I get: $> echo on > /proc/sys/kernel/printk_devkmsg $> cat /proc/sys/kernel/printk_devkmsg on $> echo -e "ratelimit\nXXXX" > /proc/sys/kernel/printk_devkmsg -bash: echo: write error: Invalid argument $> cat /proc/sys/kernel/printk_devkmsg ratelimit The second write returns error but the value is written. I am not sure where the error comes from. It seems that devkmsg_sysctl_set_loglvl() returns 0 in both cases. I wonder if my solution still would be a good deal. We could fix the remaining issue just by increasing the input buffer by one byte. Note that proc_dostring() checks for '\n'. It is the only character that it does not writte into the buffer. Then the extended strncmp() check will do exact match and find any other mispelling, including a non-newline suffix. >>From 0ce6125caf314270cb48202390d8a0938fdf316e Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Tue, 31 Jan 2017 12:00:13 +0100 Subject: [PATCH] printk: Fix printk.devkmsg sysctl The code handling write into /proc/sys/kernel/printk_devkmsg expects a new line at the end of the string but does not check it. As a result it allows: # echo -n offX > /proc/sys/kernel/printk_devkmsg # while at the same time it rejects legitimate uses: # echo -n off > /proc/sys/kernel/printk_devkmsg -sh: echo: write error: Invalid argument This patch keeps using proc_dostring() to avoid a lot of duplicate code. It increases the buffer size and allows to read one more character beyond the string "ratelimit". In addition, it increases the limit for strncmp() so that it checks also the trailing '\0' and do exact matches. Note that proc_dostring() checks for the trailing '\n' and does not write it into the buffer. As a result __control_devkmsg() is able to detect all misspelling. and could return proper error codes. Also it never sets a wrong devkmsg_log so that it need not be reverted later. The code still does the ugly devkmsg_log_str write/restore. But it looks like an acceptable deal. Reported-by: Rabin Vincent Signed-off-by: Petr Mladek --- include/linux/printk.h | 4 ++-- kernel/printk/printk.c | 33 +++++++++++---------------------- 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/include/linux/printk.h b/include/linux/printk.h index 3472cc6b7a60..c5aa0e268990 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -77,8 +77,8 @@ static inline void console_verbose(void) console_loglevel = CONSOLE_LOGLEVEL_MOTORMOUTH; } -/* strlen("ratelimit") + 1 */ -#define DEVKMSG_STR_MAX_SIZE 10 +/* strlen("ratelimit") + '\0' + one character to detect wrong entry */ +#define DEVKMSG_STR_MAX_SIZE 11 extern char devkmsg_log_str[]; struct ctl_table; diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 8b2696420abb..7c56742a8baa 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -107,17 +107,17 @@ static int __control_devkmsg(char *str) if (!str) return -EINVAL; - if (!strncmp(str, "on", 2)) { + /* Do exact match by comparing also the trailing '\0'. */ + if (!strncmp(str, "on", 3)) devkmsg_log = DEVKMSG_LOG_MASK_ON; - return 2; - } else if (!strncmp(str, "off", 3)) { + else if (!strncmp(str, "off", 4)) devkmsg_log = DEVKMSG_LOG_MASK_OFF; - return 3; - } else if (!strncmp(str, "ratelimit", 9)) { + else if (!strncmp(str, "ratelimit", 10)) devkmsg_log = DEVKMSG_LOG_MASK_DEFAULT; - return 9; - } - return -EINVAL; + else + return -EINVAL; + + return 0; } static int __init control_devkmsg(char *str) @@ -155,14 +155,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, loff_t *ppos) { char old_str[DEVKMSG_STR_MAX_SIZE]; - unsigned int old; int err; if (write) { if (devkmsg_log & DEVKMSG_LOG_MASK_LOCK) return -EINVAL; - old = devkmsg_log; strncpy(old_str, devkmsg_log_str, DEVKMSG_STR_MAX_SIZE); } @@ -173,21 +171,12 @@ 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)) { - - /* ... and restore old setting. */ - devkmsg_log = old; + /* Restore old setting when unknown parameter was used. */ + if (err) strncpy(devkmsg_log_str, old_str, DEVKMSG_STR_MAX_SIZE); - - return -EINVAL; - } } - return 0; + return err; } /* -- 1.8.5.6