Message-ID: <20231201163120.depfyngsxdiuchvc@localhost>
Date:   Fri, 1 Dec 2023 17:31:20 +0100
From:   Joel Granados <j.granados@...sung.com>
To:     Thomas Weißschuh <linux@...ssschuh.net>
CC:     Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Iurii Zaikin <yzaikin@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        <linux-hardening@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH RFC 0/7] sysctl: constify sysctl ctl_tables

Hey Thomas.

Thx for the clarifications. I did more of a deep dive into your set and
have additional comments (in line). I think const-ing all this is a good
approach. The way forward is to be able to see the entire patch set of
changes in a V1 or a shared repo somewhere to have a better picture of
what is going on. By the "entire patchset" I mean all the changes that
you described in the "full process".

On Tue, Nov 28, 2023 at 09:18:30AM +0100, Thomas Weißschuh wrote:
> Hi Joel,
> 
> On 2023-11-27 11:13:23+0100, Joel Granados wrote:
> > In general I would like to see more clarity with the motivation and I
> > would also expect some system testing. My comments inline:
> 
> Thanks for your feedback, response are below.
> 
> > On Sat, Nov 25, 2023 at 01:52:49PM +0100, Thomas Weißschuh wrote:
> > > Problem description:
> > > 
> > > The kernel contains a lot of struct ctl_table throught the tree.
> > > These are very often 'static' definitions.
> > > It would be good to mark these tables const to avoid accidental or
> > > malicious modifications.
> 
> > It is unclear to me what you mean here with accidental or malicious
> > modifications. Do you have a specific attack vector in mind? Do you
> > have an example of how this could happen maliciously? With
> > accidental, do you mean in proc/sysctl.c? Can you expand more on the
> > accidental part?
> 
> There is no specific attack vector I have in mind. The goal is to remove
> mutable data, especially if it contains pointers, that could be used by
> an attacker as a step in an exploit. See for example [0], [1].
I think you should work "remove mutable data" as part of you main
motivation when you send the non-RFC patch. I would also including [0]
and [1] (and any other previous work) to help contextualize.

> 
> Accidental can be any out-of-bounds write throughout the kernel.
> 
> > What happens with the code that modifies these outside the sysctl core?
> > Like for example in sysctl_route_net_init where the table is modified
> > depending on the net->user_ns? Would these non-const ctl_table pointers
> > be ok? would they be handled differently?
> 
> It is still completely fine to modify the tables before registering,
> like sysctl_route_net_init is doing. That code should not need any
> changes.
> 
> Modifying the table inside the handler function would bypass the
> validation done when registering so sounds like a bad idea in general.
This is done before registering. So the approach *is* sound.

> It would still be possible however for a subsystem to do so by just not
> making their sysctl table const and then modifying the table directly.
Indeed. Which might be intended or migth be someone that just forgets to
put const. I think you mentioned that there would be some sort of static
check for this (coccinelle or smach, or something else)? 

>  
> > > Unfortunately the tables can not be made const because the core
> > > registration functions expect mutable tables.
> > > 
> > > This is for two reasons:
> > > 
> > > 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
> > >    the table. This should be fixable by only modifying the header
> > >    instead of the table itself.
> > > 2) The table is passed to the handler function as a non-const pointer.
> > > 
> > > This series is an aproach on fixing reason 2).
> 
> > So number 2 will be sent in another set?
Sorry, this was supposed to be "number 1", but you got my meaning :)

> 
> If the initial feedback to the RFC and general process is positive, yes.
Off the top of my head, putting  that type in the header instead of the
ctl_table seems ok. I would include it in non-RFC version together with
2.

> 
> > > 
> > > Full process:
> > > 
> > > * Introduce field proc_handler_new for const handlers (this series)
I don't understand why we need a new handler. Couldn't we just change
the existing handler to receive `const struct ctl_table` and change all
the `proc_do*` handlers?
I'm guessing its because you want to do this in steps? if that is the
case, it would be very helpfull to see (in some repo or V1) the steps
to change all the handlers in the non-RFC version 

> > > * Migrate all core handlers to proc_handler_new (this series, partial)
> > >   This can hopefully be done in a big switch, as it only involves
> > >   functions and structures owned by the core sysctl code.
It would be helpful to see what the "big switch" would look like. If it
is all sysctl code and cannot be chunked up because of dependencies,
then it should be ok to do it in one go.

> > > * Migrate all other sysctl handlers to proc_handler_new.
> > > * Drop the old proc_handler_field.
> > > * Fix the sysctl core to not modify the tables anymore.
> > > * Adapt public sysctl APIs to take "const struct ctl_table *".
> > > * Teach checkpatch.pl to warn on non-const "struct ctl_table"
> > >   definitions.
Have you considered how to ignore the cases where the ctl_tables are
supposed to be non-const when they are defined (like in the network
code that we were discussing earlier)

> > > * Migrate definitions of "struct ctl_table" to "const" where applicable.
These migrations are treewide and are usually reviewed by a wider
audience. You might need to chunk it up to make the review more palpable
for the other maintainers.

> > >  
> > > 
> > > Notes:
> > > 
> > > Just casting the function pointers around would trigger
> > > CFI (control flow integrity) warnings.
> > > 
> > > The name of the new handler "proc_handler_new" is a bit too long messing
> > > up the alignment of the table definitions.
> > > Maybe "proc_handler2" or "proc_handler_c" for (const) would be better.
> 
> > indeed the name does not say much. "_new" looses its meaning quite fast
> > :)
> 
> Hopefully somebody comes up with a better name!
I would like to avoid this all together and just do add the const to the
existing "proc_handler"

> 
> > In my experience these tree wide modifications are quite tricky. Have you
> > run any tests to see that everything is as it was? sysctl selftests and
> > 0-day come to mind.
> 
> I managed to miss one change in my initial submission:
> With the hunk below selftests and typing emails work.
> 
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -1151,7 +1151,7 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
>                         else
>                                 err |= sysctl_check_table_array(path, entry);
>                 }
> -               if (!entry->proc_handler)
> +               if (!entry->proc_handler && !entry->proc_handler_new)
>                         err |= sysctl_err(path, entry, "No proc_handler");
>  
>                 if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode)
> 
> > [..]
> 
> [0] 43a7206b0963 ("driver core: class: make class_register() take a const *")
> [1] https://lore.kernel.org/lkml/20230930050033.41174-1-wedsonaf@gmail.com/
> 
> 
> Thomas

-- 

Best
Joel Granados

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)

Powered by blists - more mailing lists