| 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: <07477d83-2044-44c2-b334-b08d73d4da2b@linux.dev>
Date: Sat, 20 Dec 2025 02:10:17 +0800
From: Wen Yang <wen.yang@...ux.dev>
To: Joel Granados <joel.granados@...nel.org>
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 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.
--
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
>
Powered by blists - more mailing lists