Re: [PATCH ] sysctl: fix uninitialized variable in proc_do_large_bitmap

[Peter Seiderer <ps.report@gmx.net>]

Hello Marc,

On Sat, 14 Mar 2026 10:37:25 +0100, Marc Buerg <buermarc@googlemail.com> wrote:

> Hello Peter,
> 
> Thanks for your feedback and the idea. You are correct proc_get_long()
> does not set @tr if @size is zero, therefore, left in
> proc_do_large_bitmap() should be zero when we expect @tr to not be
> written to and c still being uninitialized.
> 
> > Would the better fix be:
> > 
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 354a2d294f52..89db88552987 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -1427,7 +1427,7 @@ int proc_do_large_bitmap(struct ctl_table *table, in=
> > t write,
> >  				left--;
> >  			}
> >  
> > -			if (c == '-') {
> > +			if (left && c == '-') {
> >  				err = proc_get_long(&p, &left, &val_b,
> >  						     &neg, tr_b, sizeof(tr_b),
> >  						     &c);  
> 
> This would explicitly fix the problem as it enforces that we only check
> if we know c contains what we want to check for. Fixing it like you
> proposed seems better to me.
> 
> I am somewhat conflicted because leaving c uninitialized allows that a
> similar problematic access of c could be made in the future.
> Initializing c could prevent that. I also do not see an immediate
> downside, but that could just be my naivety. Further, that part would
> now behave similar to when we apply the default hardening configuration,
> if my understanding is correct.

Your 'initialize c' (outside of the while loop) approach will only fix the
problem at the first iteration of the loop, on further iterations c will be
overwritten by the prior proc_get_long() calls..., for me the 'check c only
if valid' seems the better approach.... ;-)

Regards,
Peter

> 
> On the other hand, we do not read c later on, and I do not see a reason
> why the function would change significantly. Still, it feels more
> defensive to me to also set c to 0.
> 
> In the end, I am not so used to the kernel coding style. Is there
> anything that can be argued against providing both? If you think this is
> unnecessary I am happy to follow your reasoning and go with only the
> check for left being non-zero.
> 
> Kind Regards,
> Marc