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.
