| Message ID | 20241114162638.57392-3-nicolas.bouchinet@clip-os.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes multiple sysctl proc_handler usage error | expand |
On Thu, Nov 14, 2024 at 05:25:51PM +0100, nicolas.bouchinet@clip-os.org wrote: > From: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > > Commit 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in > vm_table") fixes underflow value setting risk in vm_table but misses > vdso_enabled sysctl. > > vdso_enabled sysctl is initialized with .extra1 value as SYSCTL_ZERO to > avoid negative value writes but the proc_handler is proc_dointvec and not > proc_dointvec_minmax and thus do not uses .extra1 and .extra2. > > The following command thus works : > > `# echo -1 > /proc/sys/vm/vdso_enabled` It would be interesting to know what happens when you do a # echo (INT_MAX + 1) > /proc/sys/vm/vdso_enabled This is the reasons why I'm interested in such a test: 1. Both proc_dointvec and proc_dointvec_minmax (calls proc_dointvec) have a overflow check where they will return -EINVAL if what is given by the user is greater than (unsiged long)INT_MAX; this will evaluate can evaluate to true or false depending on the architecture where we are running. 2. I noticed that vdso_enabled is an unsigned long. And so the expectation is that the range is 0 to ULONG_MAX, which in some cases (depending on the arch) would not be the case. So my question is: What is the expected range for this value? Because you might not be getting the whole range in the cases where int is 32 bit and long is 64 bit. > > This patch properly sets the proc_handler to proc_dointvec_minmax. > > Fixes: 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in vm_table") > Signed-off-by: Nicolas Bouchinet <nicolas.bouchinet@ssi.gouv.fr> > --- > kernel/sysctl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 79e6cb1d5c48f..37b1c1a760985 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2194,7 +2194,7 @@ static struct ctl_table vm_table[] = { > .maxlen = sizeof(vdso_enabled), > #endif > .mode = 0644, > - .proc_handler = proc_dointvec, > + .proc_handler = proc_dointvec_minmax, > .extra1 = SYSCTL_ZERO, Any reason why extra2 is not defined. I know that it was not defined before, but this does not mean that it will not have an upper limit. The way that I read the situation is that this will be bounded by the overflow check done in proc_dointvec and will have an upper limit of INT_MAX. Please correct me if I have read the situation incorrectly. Best
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 79e6cb1d5c48f..37b1c1a760985 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2194,7 +2194,7 @@ static struct ctl_table vm_table[] = { .maxlen = sizeof(vdso_enabled), #endif .mode = 0644, - .proc_handler = proc_dointvec, + .proc_handler = proc_dointvec_minmax, .extra1 = SYSCTL_ZERO, }, #endif