linux-kernel - Re: [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f250814f-2c04-7bbf-035d-1dedcb260335@suse.cz>
Date:   Thu, 26 Mar 2020 10:30:02 +0100
From:   Vlastimil Babka <vbabka@...e.cz>
To:     Kees Cook <keescook@...omium.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        linux-kernel@...r.kernel.org, linux-api@...r.kernel.org,
        linux-mm@...ck.org, Ivan Teterevkov <ivan.teterevkov@...anix.com>,
        Michal Hocko <mhocko@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Matthew Wilcox <willy@...radead.org>,
        "Eric W . Biederman" <ebiederm@...ssion.com>,
        "Guilherme G . Piccoli" <gpiccoli@...onical.com>
Subject: Re: [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters
 from kernel command line

On 3/25/20 10:21 PM, Kees Cook wrote:
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1345,6 +1345,25 @@ void __weak free_initmem(void)
>>  	free_initmem_default(POISON_FREE_INITMEM);
>>  }
>>  
>> +static void do_sysctl_args(void)
>> +{
>> +#ifdef CONFIG_SYSCTL
>> +	size_t len = strlen(saved_command_line) + 1;
>> +	char *command_line;
>> +
>> +	command_line = kzalloc(len, GFP_KERNEL);
>> +	if (!command_line)
>> +		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
>> +
>> +	strcpy(command_line, saved_command_line);
> 
> No need to open-code this:
> 
> 	char *command_line;
> 
> 	command_line = kstrdup(saved_command_line, GFP_KERNEL);
> 	if (!command_line)
> 		panic("%s: Failed to allocate %zu bytes\n", __func__, len);
> 

Ah, right. I admit I basically copy_pasted some other parse_args user.

>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index ad5b88a53c5a..18c7f5606d55 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1980,6 +1980,68 @@ int __init sysctl_init(void)
>>  	return 0;
>>  }
>>  
>> +/* Set sysctl value passed on kernel command line. */
>> +int process_sysctl_arg(char *param, char *val,
>> +			       const char *unused, void *arg)
>> +{
>> +	size_t count;
>> +	char *remaining;
>> +	int err;
>> +	loff_t ppos = 0;
>> +	struct ctl_table *ctl, *found = NULL;
>> +
>> +	if (strncmp(param, "sysctl.", sizeof("sysctl.") - 1))
>> +		return 0;
>> +
>> +	param += sizeof("sysctl.") - 1;
>> +
>> +	remaining = param;
>> +	ctl = &sysctl_base_table[0];
>> +
>> +	while(ctl->procname != 0) {
>> +		int len = strlen(ctl->procname);
>> +		if (strncmp(remaining, ctl->procname, len)) {
>> +			ctl++;
>> +			continue;
>> +		}
> 
> I think you need to validate that "len" is within "remaining" here
> first.

My reasoning was that if remaining terminates too early, the null byte would be
different from non-null byte in ctl->procname and thus strncmp will return it as
different?
And the reason I used len in strncmp there is only so it doesn't compare the
terminating null, because remaning can continue with ".foo" instead.

>> +		if (ctl->child) {
>> +			if (remaining[len] == '.') {
>> +				remaining += len + 1;
> 
> And that "len + 1" is still valid.

And since we passed strncmp(..., len), remaining[len] might be null byte, but
then we can still compare it with '.'.

But C strings are full of landmines.