| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7mevcgca7xsd5gckrap22byjinhrgqnelu4y7hzo2r5t2cq7w4@nyaa42khwqez>
Date: Mon, 22 Dec 2025 22:59:54 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Wen Yang <wen.yang@...ux.dev>
Cc: "Eric W. Biederman" <ebiederm@...ssion.com>,
Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <keescook@...omium.org>,
Christian Brauner <brauner@...nel.org>, Dave Young <dyoung@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5] sysctl: simplify the min/max boundary check
On Sat, Dec 20, 2025 at 02:10:17AM +0800, Wen Yang wrote:
>
>
> On 12/19/25 22:44, Joel Granados wrote:
> > Hey Wen
> >
> > I know it has been a very long time, but I think I have a plan to make
> > this happen. And by "this" I mean changing the extra{1,2} to another
> > type. I'm writing to you to see if you are still up for pushing it
> > forward.
> >
> > My proposal would go in two big steps. Each one might take more than one
> > release.
> >
> > First step
> > ==========
> > Consolidate the way ctl_table entries are created into a SYSCTL_ENTRY
> > macro. There would be no functional change on this first step. It is
> > just a preparation step for the second stage. The main objective is to
> > put all the logic of creating the ctl_table entry in one place (mainly
> > into sysctl.h). An example of what I mean for step one in [1]
> >
> > Second step
> > ===========
> > Actually change the extern{1,2} from *void to ulong. Having the logic
> > in one place makes this easier. Here I'm guessing that we would still
> > have to handle outliers (like net), but they would be much less.
> >
> > In my opinion, if we are going to go through the pain of doing a
> > treewide change, we might as well make it easier for ourselves to make
> > systemic changes to sysctl in the future; we do this by having the
> > creation macro within sysctl.{c,h}
> >
> > Tell me if you are still interested and have cycles for interested
> >
>
> Thank you very much.
> It is my honor to participate in this program, and I am willing to join as
> soon as possible.
That is fantastic. Here are two suggestions of what you can move on:
1. The place were I left off in [1] was with NEIGH_SYSCTL_ENTRY. You can
pick it up from there by making NEIGH_SYSCTL_ENTRY work with the
SYSCTL_*_ENTRY macros in [1].
2. Create a coccinelle script that will change all the ctl_table entries
that fit the new SYSCTL_*_ENTRY macros so that the changes happen
automatically.
Communication
=============
Please do **not** send a huge series with all the changes that you
find. I suggest that we start discussing the changes between us and then
share an RFC with the greater community once we are happy. With this in
mind, I suggest we remove Dave, Eric, Luis, Kees and Christian from the
CC (unless they want to tag along for the ride) and leave the list so we
have the discussion in public.
Be back in Jan
==============
I'm on hollidays ATM and will be back in full second week of Jan.
Get back to me if any of this does not make sense :)
Best
>
> --
> Best wishes,
> Wen
>
> > Best
> >
> > [1]
> >
> > diff --git i/include/linux/sysctl.h w/include/linux/sysctl.h
> > index 39cf1bf9703f..716ad7e41a01 100644
> > --- i/include/linux/sysctl.h
> > +++ w/include/linux/sysctl.h
> > @@ -58,6 +58,7 @@ extern const int sysctl_vals[];
> > #define SYSCTL_LONG_ZERO ((void *)&sysctl_long_vals[0])
> > #define SYSCTL_LONG_ONE ((void *)&sysctl_long_vals[1])
> > #define SYSCTL_LONG_MAX ((void *)&sysctl_long_vals[2])
> > +extern const void *SYSCTL_NULL;
> > /**
> > *
> > @@ -230,6 +231,23 @@ struct ctl_table {
> > void *extra2;
> > } __randomize_layout;
> > +#define __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, HANDLER, SMIN, SMAX)\
> > + { \
> > + .procname = NAME, \
> > + .data = (void*) &DATA, \
> > + .maxlen = sizeof(TYPE), \
> > + .mode = MODE, \
> > + .proc_handler = HANDLER, \
> > + .extra1 = SMIN, \
> > + .extra2 = SMAX, \
> > + }
> > +
> > +#define SYSCTL_ENTRY(NAME, DATA, TYPE, MODE) \
> > + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec, NULL, NULL)
> > +
> > +#define SYSCTL_RANGE_ENTRY(NAME, DATA, TYPE, MODE, SMIN, SMAX) \
> > + __SYSCTL_ENTRY(NAME, DATA, TYPE, MODE, proc_do##TYPE##vec_minmax, SMIN, SMAX)
> > +
> > struct ctl_node {
> > struct rb_node node;
> > struct ctl_table_header *header;
> > diff --git i/kernel/exit.c w/kernel/exit.c
> > index 8a87021211ae..04e531e45912 100644
> > --- i/kernel/exit.c
> > +++ w/kernel/exit.c
> > @@ -88,13 +88,7 @@ static unsigned int oops_limit = 10000;
> > #ifdef CONFIG_SYSCTL
> > static const struct ctl_table kern_exit_table[] = {
> > - {
> > - .procname = "oops_limit",
> > - .data = &oops_limit,
> > - .maxlen = sizeof(oops_limit),
> > - .mode = 0644,
> > - .proc_handler = proc_douintvec,
> > - },
> > + SYSCTL_ENTRY("oops_limit", oops_limit, uint, 0644),
> > };
> > static __init int kernel_exit_sysctls_init(void)
> > diff --git i/kernel/sysctl.c w/kernel/sysctl.c
> > index 0965ea1212b4..fa228b89862a 100644
> > --- i/kernel/sysctl.c
> > +++ w/kernel/sysctl.c
> > @@ -22,6 +22,7 @@
> > #include <linux/uaccess.h>
> > #include <asm/processor.h>
> > +const void *SYSCTL_NULL = NULL;
> > /* shared constants to be used in various sysctls */
> > const int sysctl_vals[] = { 0, 1, 2, 3, 4, 100, 200, 1000, 3000, INT_MAX, 65535, -1 };
> > EXPORT_SYMBOL(sysctl_vals);
> > @@ -1328,47 +1329,16 @@ int proc_do_static_key(const struct ctl_table *table, int dir,
> > static const struct ctl_table sysctl_subsys_table[] = {
> > #ifdef CONFIG_PROC_SYSCTL
> > - {
> > - .procname = "sysctl_writes_strict",
> > - .data = &sysctl_writes_strict,
> > - .maxlen = sizeof(int),
> > - .mode = 0644,
> > - .proc_handler = proc_dointvec_minmax,
> > - .extra1 = SYSCTL_NEG_ONE,
> > - .extra2 = SYSCTL_ONE,
> > - },
> > + SYSCTL_RANGE_ENTRY("sysctl_writes_strict", sysctl_writes_strict, int, \
> > + 0644, SYSCTL_NEG_ONE, SYSCTL_ONE),
> > #endif
> > - {
> > - .procname = "ngroups_max",
> > - .data = (void *)&ngroups_max,
> > - .maxlen = sizeof (int),
> > - .mode = 0444,
> > - .proc_handler = proc_dointvec,
> > - },
> > - {
> > - .procname = "cap_last_cap",
> > - .data = (void *)&cap_last_cap,
> > - .maxlen = sizeof(int),
> > - .mode = 0444,
> > - .proc_handler = proc_dointvec,
> > - },
> > + SYSCTL_ENTRY("ngroups_max", ngroups_max, int, 0444),
> > + SYSCTL_ENTRY("cap_last_cap", cap_last_cap, int, 0444),
> > #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_ALLOW
> > - {
> > - .procname = "unaligned-trap",
> > - .data = &unaligned_enabled,
> > - .maxlen = sizeof(int),
> > - .mode = 0644,
> > - .proc_handler = proc_dointvec,
> > - },
> > + SYSCTL_ENTRY("unaligned-trap", unaligned_enabled, int, 0644),
> > #endif
> > #ifdef CONFIG_SYSCTL_ARCH_UNALIGN_NO_WARN
> > - {
> > - .procname = "ignore-unaligned-usertrap",
> > - .data = &no_unaligned_warning,
> > - .maxlen = sizeof (int),
> > - .mode = 0644,
> > - .proc_handler = proc_dointvec,
> > - },
> > + SYSCTL_ENTRY("ignore-unaligned-usertrap", no_unaligned_warning, int, 0644),
> > #endif
> > };
> > diff --git i/kernel/ucount.c w/kernel/ucount.c
> > index 586af49fc03e..b9f4fec7638a 100644
> > --- i/kernel/ucount.c
> > +++ w/kernel/ucount.c
> > @@ -63,15 +63,9 @@ static struct ctl_table_root set_root = {
> > static long ue_zero = 0;
> > static long ue_int_max = INT_MAX;
> > -#define UCOUNT_ENTRY(name) \
> > - { \
> > - .procname = name, \
> > - .maxlen = sizeof(long), \
> > - .mode = 0644, \
> > - .proc_handler = proc_doulongvec_minmax, \
> > - .extra1 = &ue_zero, \
> > - .extra2 = &ue_int_max, \
> > - }
> > +#define UCOUNT_ENTRY(name) \
> > + SYSCTL_RANGE_ENTRY(name, SYSCTL_NULL, ulong, 0644, &ue_zero, &ue_int_max)
> > +
> > static const struct ctl_table user_table[] = {
> > UCOUNT_ENTRY("max_user_namespaces"),
> > UCOUNT_ENTRY("max_pid_namespaces"),
> >
> > On Thu, Feb 27, 2025 at 03:09:12PM +0100, Joel Granados wrote:
> > > On Thu, Jan 30, 2025 at 10:32:14PM +0800, Wen Yang wrote:
> > > >
> > > >
> > > > On 2025/1/28 01:51, Eric W. Biederman wrote:
> > > > > Joel Granados <joel.granados@...nel.org> writes:
> > > > >
> > > ...
> > > > > that use extra1 or extra2 for something besides min and max. Then
> > > > > remove extra1 and extra2. At the end it is simpler and requires the
> > > > > same or a little less space.
> > > > >
> > > > > That was and remains my suggestion.
> > > > >
> > > >
> > > > Thanks for your valuable suggestions. We will continue to move forward along
> > > > it and need your more guidance.
> > > >
> > > > But there are also a few codes that do take the extra{1, 2} as pointers, for
> > > > example:
> > > >
> > > > int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
> > > > proc_handler *handler)
> > > > {
> > > > ...
> > > > for (i = 0; i < NEIGH_VAR_GC_INTERVAL; i++) {
> > > > t->neigh_vars[i].data += (long) p;
> > > > t->neigh_vars[i].extra1 = dev;
> > > > t->neigh_vars[i].extra2 = p;
> > > > }
> > > > ...
> > > > }
> > > >
> > > > static void neigh_proc_update(const struct ctl_table *ctl, int write)
> > > > {
> > > > struct net_device *dev = ctl->extra1;
> > > > struct neigh_parms *p = ctl->extra2;
> > > > struct net *net = neigh_parms_net(p);
> > > > int index = (int *) ctl->data - p->data;
> > > > ...
> > > > }
> > > >
> > > >
> > > > So could we modify it in this way to make it compatible with these two
> > > > situations:
> > > >
> > > > @@ -137,8 +137,16 @@ struct ctl_table {
> > > > umode_t mode;
> > > > proc_handler *proc_handler; /* Callback for text formatting */
> > > > struct ctl_table_poll *poll;
> > > > - void *extra1;
> > > > - void *extra2;
> > > > + union {
> > > > + struct {
> > > > + void *extra1;
> > > > + void *extra2;
> > > > + };
> > > > + struct {
> > > > + unsigned long min;
> > > > + unsigned long max;
> > > > + };
> > > > + };
> > > > } __randomize_layout;
> > >
> > > I'm still not convinced that a union is the best way out of this. I have
> > > postponed reviewing this for several weeks, but I'm slowly coming back
> > > to it.
> > >
> > > Thx for your suggestion
> > >
> > > Best
> > >
> > >
> > > --
> > >
> > > Joel Granados
> >
--
Joel Granados
Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)
Powered by blists - more mailing lists