Hi Joel,
Thank's for your reply.
I apologize for the reply delay, I wasn't available late weeks.
On 11/20/24 1:53 PM, Joel Granados wrote:
On Thu, Nov 14, 2024 at 05:25:51PM +0100, nicolas.bouchinet@xxxxxxxxxxx wrote:
From: Nicolas Bouchinet <nicolas.bouchinet@xxxxxxxxxxx>
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
Great question, I'll check that.
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.
Indeed, I'll run tests to avouch behaviors of proc handlers bound checks
with
different architectures.
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.
Yep, it is. As I've tried to explain in the cover letter
(https://lore.kernel.org/all/20241112131357.49582-1-nicolas.bouchinet@xxxxxxxxxxx/),
there are numerous places where sysctl data type differs from the proc
handler
return type.
AFAIK, for proc_dointvec there is more than 10 different sysctl where it
happens. The three I've patched represents three common mistakes using
proc_handlers.
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@xxxxxxxxxxx>
---
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.
Yes, it is bounded by the overflow checks done in proc_dointvec, I've not
changed the current sysctl behavior but we should bound it between 0
and 1 since it seems vdso compat is not supported anymore since
Commit b0b49f2673f011cad ("x86, vdso: Remove compat vdso support").
This is the behavior of vdso32_enabled exposed under the abi sysctl
node.
Please correct me if I have read the situation incorrectly.
You perfectly understood the problematic of it, thanks a lot for your
review.
I'll reply to above questions after I've run more tests.
I saw GKH already merged the third commit of this patchset and
backported it to stable branches.
Should I evict it from future version of this patchset ?
Thanks,
Nicolas
Best
