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

[Marc Buerg <buermarc@googlemail.com>]

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.

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