lkml.org 
[lkml]   [2020]   [Mar]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v2 1/2] kernel/sysctl: support setting sysctl parameters from kernel command line
    From
    Date
    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.

    \
     
     \ /
      Last update: 2020-03-26 10:30    [W:4.122 / U:0.256 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site