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@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. 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 -- Joel Granados
