Re: [PATCH v3] sysctl: fix check against uninitialized variable in proc_do_large_bitmap

[Joel Granados]

On Tue, Mar 24, 2026 at 11:36:40PM +0100, buermarc wrote:
> From: Marc Buerg <buermarc@xxxxxxxxxxxxxx>
> 
> Hi,
> 
> On Tue, 24 Mar 2026 08:44:13 +0100 Joel Granados, wrote:
> 
> > 1. len is calculated by doing len = p - tmp
> > 2. tmp is the beginning of the buffer
> > 3. p is the first character that is not a digit.
> >
> > There will always be at least one ('\0') character that is not a digit.
> 
> Yes.
> 
> > So len will be less than size at least by one (could be more if the
> > string contains non digit chars). When it is parsed to the end,
> > len will be less than size by one.
> 
> I don't think this is the case. See below.
> 
> > TL;DR
> 
> I'll try to make it shorter. size is the length of the user buffer, even
> after the copy to the kernel buffer. The added '\0' does not increment
> size in proc_sys_call_handler(). Following the example for
> write(fd,"123",3). In the beginning of proc_get_long():
> 
> size = 3
> 
> @:012 3
> -------
>  "123\0"
>   |
>  tmp=@0
>   |
>   p=@0
> 
> After strtoul_lenient():
> 
> @:012 3
> -------
>  "123\0"
>   |   |
>  tmp=@0
>       |
>       p=@3
> 
> @3 - @0 = 3
> len = 3
> 
> (len < size) => (3 < 3) => false

I see it now! I needed to pass '-n' to echo. I was testing with
write(fd, "123\n" 4) instead of write(fd, "123", 3)

> 
> And size must be 3, proc_sys_call_handler() does not increment it. If we
> provide something like write(fd,"123\0",4) size will be 4, but now we will
> return -EINVAL.
Agreed.

> 
> If len < size is true, we call memchk(). memchk will not find a trailing
> character as p points to '\0'. This means in this case we would return
> -EINVAL for the first proc_get_long() call in the while loop. Reason is
> '\0' is not in tr_a and therefore not a valid delimiter for the first
> proc_get_long() call.
> 
> I hope this get's my point across.
It does. Thx for insisting :)

> 
> > All this holds unless the trailing '\0' is modified in some way in that
> > BPF call.
> 
> To my knowledge there was no bpf tracing enabled on the affected host at
> the time that the behavior was observed.
Indeed. I can reproduce without BPF

> 
> Finally: the fix which adds a check for left introduces a regression.
> "123-" is wrongly accepted. Not sure if you saw my other message. It
> should not be used.
I saw that, thx.

I think the correct fix is your original version. Init 'c' to zero only.
Please resend it without the '---' so I can pull your trailers properly.

We don't check 'left' in the c == '-' condition because it will
incorrectly accept strings that end in '-' (e.g. "123-"). In other
words, we need to enter that branch to fail as expected.

> 
> Best Regards,
> Marc

Thx

-- 

Joel Granados

Attachment: signature.asc
Description: PGP signature