| 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: <sytyqb3ajm6ysoifwp57ga7gzlnzodhdsbgizbn3hqnlwytn5a@pbscftrq2qwl>
Date: Fri, 19 Dec 2025 15:44:44 +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
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
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