Message-ID: <202004031654.C4389A04EF@keescook>
Date:   Fri, 3 Apr 2020 16:57:51 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     Vlastimil Babka <vbabka@...e.cz>,
        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>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Christian Brauner <christian.brauner@...ntu.com>
Subject: Re: [PATCH 1/3] kernel/sysctl: support setting sysctl parameters
 from kernel command line

On Thu, Apr 02, 2020 at 08:59:32PM +0000, Luis Chamberlain wrote:
> On Thu, Apr 02, 2020 at 10:23:13AM -0700, Kees Cook wrote:
> > On Thu, Apr 02, 2020 at 04:04:42PM +0000, Luis Chamberlain wrote:
> > > On Wed, Apr 01, 2020 at 01:01:47PM +0200, Vlastimil Babka wrote:
> > > > On 3/31/20 12:44 AM, Luis Chamberlain wrote:
> > > > >> +	} else if (wret != len) {
> > > > >> +		pr_err("Wrote only %ld bytes of %d writing to proc file %s to set sysctl parameter '%s=%s'",
> > > > >> +			wret, len, path, param, val);
> > > > >> +	}
> > > > >> +
> > > > >> +	err = filp_close(file, NULL);
> > > > >> +	if (err)
> > > > >> +		pr_err("Error %pe closing proc file to set sysctl parameter '%s=%s'",
> > > > >> +			ERR_PTR(err), param, val);
> > > > >> +out:
> > > > >> +	kfree(path);
> > > > >> +	return 0;
> > > > >> +}
> > > > >> +
> > > > >> +void do_sysctl_args(void)
> > > > >> +{
> > > > >> +	char *command_line;
> > > > >> +	struct vfsmount *proc_mnt = NULL;
> > > > >> +
> > > > >> +	command_line = kstrdup(saved_command_line, GFP_KERNEL);
> > > > > 
> > > > > can you use kstrndup() ? And then use kfree_const()? Yes, feel free to
> > > > 
> > > > I don't follow, what am I missing? Do you mean this?
> > > > 
> > > > size_t len = strlen(saved_command_line);
> > > > command_line = kstrndup(saved_command_line, len, GFP_KERNEL);
> > > > 
> > > > What would be the advantage over plain kstrdup()?
> > > > As for kfree_const(), when would command_line be .rodata? I don't see using
> > > > kstrndup() resulting in that.
> > > 
> > > The const nature of using kstrdup() comes with using const for your
> > > purpose. ie:
> > > 
> > > const char *const_command_line = saved_command_line;
> > > 
> > > The point of a kstrncpy() then is to ensure force a const throughout
> > > your use if you know you don't need modifications.
> > 
> > I'm not following this suggestion. It _is_ modifying it. That's why it's
> > making a copy. What am I missing?
> 
> We modify the copied bootparams to allow new sysctls to map to old boot params?
> 
> If so, then yes, this cannot be used.

I feel like I've lost track of this thread. This strdup is so that the
command line can have '\0's injected while it steps through the args
(and for doing the . and / replacement). I don't know what you mean by
"map" here: this is standard parse_args() usage.

> > > > >> +	parse_args("Setting sysctl args", command_line,
> > > > >> +		   NULL, 0, -1, -1, &proc_mnt, process_sysctl_arg);
> > > > >> +
> > > > >> +	if (proc_mnt)
> > > > >> +		kern_unmount(proc_mnt);
> > > > >> +
> > > > >> +	kfree(command_line);
> > > > >> +}
> > > > > 
> > > > > Then, can we get this tested as part of lib/test_sysctl.c with its
> > > > > respective tools/testing/selftests/sysctl/sysctl.sh ?
> > > > 
> > > > Hmm so I add some sysctl to the test "module" (in fact the 'config' file says it
> > > > should be build with 'y', which would be needed anyway) and expand the test
> > > > instructions so that the test kernel boot has to include it on the command line,
> > > > and then I verify it has been set? Or do you see a better way?
> > > 
> > > We don't necessarily have a way to test the use boot params today.
> > > That reveals an are which we should eventually put some focus on
> > > in the future. In the meantime we have to deal with what we have.
> > > 
> > > So let's think about this:
> > > 
> > > You are adding a new cmdline sysctl boot param, and also a wrapper
> > > for those old boot bootparams to also work using both new sysctl
> > > path and old path. Testing just these both should suffice.
> > > 
> > > How about this:
> > > 
> > > For testing the new feature you are adding, can you extend the default
> > > boot params *always* if a new CONFIG_TEST_SYSCTL_CMDLINE is set? Then
> > > upon boot we can verify the proc handlers for these new boot params got
> > > kicked, and likewise some other proc handlers which also can be used
> > > from the cmdline are *not* set. For this later set, we already have
> > > a series of test syctls you can use. In fact, you can use the existing
> > > syctls for both cases already I believe, its just a matter of adding
> > > this new CONFIG_TEST_SYSCTL_CMDLINE which would extend the cmdline,
> > > and these tests would take place *first* on the script.
> > 
> > This seems... messy.
> 
> It is all we have.
> > I'm all for testing this,
> 
> OK so we do want to test it.
> 
> > but I'd rather this not be internally driven.
> 
> This is the least cumbersome solution I could think of. Other things
> would require things like using qemu, etc. That seems much more messsy.

Yes. Doing an internal extension isn't testing the actual code.

> 
> > This is an external interface (boot params), so
> > I'd rather an external driver handle that testing. We don't have a
> > common method to do that with the kernel, though.
> 
> Right... which begs the question now -- how do we test this sort of
> stuff? The above would at least get us coverage while we iron something
> more generic out for boot params.
> 
> > > That would test both cases with one kernel.
> > > 
> > > You could then also add a bogus new sysctl which also expands to a silly
> > > raw boot param to test the wrapper you are providing. That would be the
> > > only new test syctl you would need to add.
> > 
> > Sure, that seems reasonable. Supporting externally driven testing makes
> > sense for this.
> 
> But again, what exactly?

I don't think anything is needed for this series. It can be boot tested
manually.

-- 
Kees Cook

Powered by blists - more mailing lists