Hi Joel,
I've pushed patchset version 3 :
https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@xxxxxxxxxxx/.
On 11/20/24 13:53, 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
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.
From my observations, vdso_enabled is a unsigned int. If one wants to
convert to an unsigned long, proc_doulongvec_minmax should be used
instead.
IMHO, the main issues are that .data variable type can differ from the
return
type of .proc_handler function. This can lead to undefined behaviors and
eventually vulnerabilities.
.extra1 and .extra2 can also be used with proc_handlers that do not uses
them.
I think sysctl_check_table() could be enhanced to control this behavior.
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.
If proc_dointvec or its derivative is used, as you said, range is bounded
by checks in do_proc_dointvec_conv ((unsigned long) INT_MAX).
INT_MAX being based on the max value of an int (((int)(~0U >> 1))),
do_proc_dointvec_conv behavior is thus architecture dependent.
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.
I've added an extra2 parameter to restrict vdso_enabled between 0 and 1
in patchset v3.
https://lore.kernel.org/all/20241217132908.38096-3-nicolas.bouchinet@xxxxxxxxxxx/
Please correct me if I have read the situation incorrectly.
Best
Thanks again for your review,
Best regards,
Nicolas
