| 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
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@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 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@clip-os.org/), 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@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. 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 >
Hi Joel, I've pushed patchset version 3 : https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@clip-os.org/. On 11/20/24 13:53, Joel Granados wrote: > 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. 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@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. 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@clip-os.org/ > > Please correct me if I have read the situation incorrectly. > > Best > Thanks again for your review, Best regards, Nicolas
On Tue, Dec 10, 2024 at 03:58:41PM +0100, Nicolas Bouchinet wrote: > 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@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 > > 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@clip-os.org/), > 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. It would be useful to analyze the others. Do you have more outstanding patches for these? > > > > > 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. > > 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"). I think you have already done this in your V3 > > 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 ? Yes. You should remove what has already been merged into main line. thx. Best
On Tue, Dec 17, 2024 at 02:57:51PM +0100, Nicolas Bouchinet wrote: > Hi Joel, > > I've pushed patchset version 3 : > https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@clip-os.org/. > > On 11/20/24 13:53, Joel Granados wrote: > > 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. > 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. Yep, 100% agree, I miss-read and commented incorrectly. Just ignore my previous comment; I don't know what I was smoking... > > 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. I totally agree that it can lead to unexpected behavior. Would have to look at a specific case to see if it is really "undefined". > > .extra1 and .extra2 can also be used with proc_handlers that do not > uses them. In this case they are just silently ignored. Leading the developer to believe that they are range checked, when they are really not. > I think sysctl_check_table() could be enhanced to control > this behavior. This might be the case. I can review a proposal if you send it out. 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