Re: [PATCH bpf] libbpf: Fix signedness confusion when using libbpf_is_mem_zeroed()

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Tue, Dec 13, 2022 at 5:01 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>>
>> The commit in the Fixes tag refactored the check for zeroed memory in
>> libbpf_validate_opts() into a separate libbpf_is_mem_zeroed() function.
>> This function has a 'len' argument of the signed 'ssize_t' type, which in
>> both callers is computed by subtracting two unsigned size_t values from
>> each other. In both subtractions, one of the values being subtracted is
>> converted to 'ssize_t', while the other stays 'size_t'.
>>
>> The problem with this is that, because both sizes are the same
>> rank ('ssize_t' is defined as 'long' and 'size_t' is 'unsigned long'), the
>> type of the mixed-sign arithmetic operation ends up being converted back to
>> unsigned. This means it can underflow if the user-specified size in
>> opts->sz is smaller than the size of the type as defined by libbpf. If that
>> happens, it will cause out-of-bounds reads in libbpf_is_mem_zeroed().
>
> hmm... but libbpf_is_mem_zeroed expects signed ssize_t, so that
> "underflow" will turn into a proper negative ssize_t value. What am I
> missing? Seems to be working fine:
>
> $ cat test.c
> #include <stdio.h>
>
> void testit(ssize_t sz)
> {
>         printf("%zd\n", sz);
> }
>
> int main()
> {
>         ssize_t slarge = 100;
>         size_t ularge = 100;
>         ssize_t ssmall = 50;
>         size_t usmall = 50;
>
>         testit(ssmall - slarge);
>         testit(ssmall - ularge);
>         testit(usmall - slarge);
>         testit(usmall - ularge);
> }
>
> $ cc test.c && ./a.out
> -50
> -50
> -50
> -50

Hmnm, yeah, you're right. Not sure how I managed to convince myself
there was an actual bug there :(

Sorry for the noise!

-Toke