From: Kees Cook <keescook@chromium.org>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: Vlastimil Babka <vbabka@suse.cz>,
Iurii Zaikin <yzaikin@google.com>,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
linux-mm@kvack.org, Ivan Teterevkov <ivan.teterevkov@nutanix.com>,
Michal Hocko <mhocko@kernel.org>,
David Rientjes <rientjes@google.com>,
Matthew Wilcox <willy@infradead.org>,
"Eric W . Biederman" <ebiederm@xmission.com>,
"Guilherme G . Piccoli" <gpiccoli@canonical.com>,
Alexey Dobriyan <adobriyan@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Christian Brauner <christian.brauner@ubuntu.com>
Subject: Re: [PATCH 1/3] kernel/sysctl: support setting sysctl parameters from kernel command line
Date: Fri, 3 Apr 2020 16:57:51 -0700 [thread overview]
Message-ID: <202004031654.C4389A04EF@keescook> (raw)
In-Reply-To: <20200402205932.GM11244@42.do-not-panic.com>
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
next prev parent reply other threads:[~2020-04-03 23:57 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-30 11:55 [PATCH 0/3] support setting sysctl parameters from kernel command line Vlastimil Babka
2020-03-30 11:55 ` [PATCH 1/3] kernel/sysctl: " Vlastimil Babka
2020-03-30 16:23 ` Vlastimil Babka
2020-03-30 17:40 ` Kees Cook
2020-03-30 17:39 ` Kees Cook
2020-03-30 22:44 ` Luis Chamberlain
2020-03-31 7:42 ` Vlastimil Babka
2020-03-31 7:48 ` Michal Hocko
2020-03-31 18:26 ` Kees Cook
2020-03-31 14:31 ` Luis Chamberlain
2020-04-01 11:01 ` Vlastimil Babka
2020-04-02 16:04 ` Luis Chamberlain
2020-04-02 17:23 ` Kees Cook
2020-04-02 20:59 ` Luis Chamberlain
2020-04-03 23:57 ` Kees Cook [this message]
2020-04-06 14:08 ` Luis Chamberlain
2020-04-06 15:58 ` Kees Cook
2020-04-06 17:08 ` Luis Chamberlain
2020-04-14 11:25 ` Vlastimil Babka
2020-04-15 3:23 ` Masami Hiramatsu
2020-04-15 6:08 ` Luis Chamberlain
2020-03-30 11:55 ` [PATCH 2/3] kernel/sysctl: support handling command line aliases Vlastimil Babka
2020-03-30 17:41 ` Kees Cook
2020-03-31 14:35 ` Luis Chamberlain
2020-03-30 11:55 ` [PATCH 3/3] kernel/hung_task convert hung_task_panic boot parameter to sysctl Vlastimil Babka
2020-03-30 17:43 ` Kees Cook
2020-03-31 0:34 ` John Hubbard
2020-03-31 7:27 ` Vlastimil Babka
2020-03-31 15:49 ` John Hubbard
2020-03-31 23:12 ` Tetsuo Handa
2020-04-01 8:47 ` Vlastimil Babka
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202004031654.C4389A04EF@keescook \
--to=keescook@chromium.org \
--cc=adobriyan@gmail.com \
--cc=christian.brauner@ubuntu.com \
--cc=ebiederm@xmission.com \
--cc=gpiccoli@canonical.com \
--cc=gregkh@linuxfoundation.org \
--cc=ivan.teterevkov@nutanix.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcgrof@kernel.org \
--cc=mhocko@kernel.org \
--cc=rientjes@google.com \
--cc=tglx@linutronix.de \
--cc=vbabka@suse.cz \
--cc=willy@infradead.org \
--cc=yzaikin@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).