Re: [PATCH] bpftool: Fix undefined behavior caused by shifting into the sign bit

[Kuan-Wei Chiu]

On Sun, Sep 08, 2024 at 08:48:40PM +0100, Quentin Monnet wrote:
> On 08/09/2024 15:00, Kuan-Wei Chiu wrote:
> > Replace shifts of '1' with '1U' in bitwise operations within
> > __show_dev_tc_bpf() to prevent undefined behavior caused by shifting
> > into the sign bit of a signed integer. By using '1U', the operations
> > are explicitly performed on unsigned integers, avoiding potential
> > integer overflow or sign-related issues.
> > 
> > Signed-off-by: Kuan-Wei Chiu <visitorckw@xxxxxxxxx>
> 
> 
> Looks good, thank you.
> 
> Acked-by: Quentin Monnet <qmo@xxxxxxxxxx>
> 
> How did you find these?

TL;DR: I discovered this issue through code review.

I am a student developer trying to contribute to the Linux kernel. I
was attempting to compile bpftool with ubsan enabled, and while running
./bpftool net list, I encountered the following error message:

net.c:827:2: runtime error: null pointer passed as argument 1, which is declared to never be null

This prompted me to review the code in net.c, and during that process,
I unexpectedly came across the bug that this patch addresses.

As for the ubsan complaint mentioned above, it was triggered because
qsort is being called as qsort(NULL, 0, ...) when netfilter has no
entries to display. In glibc, qsort is marked with __nonnull ((1, 4)).
However, I found conflicting information on cppreference.com [1], which
states that when count is zero, both ptr and comp can be NULL. This
confused me, so I will need to check the C standard to clarify this. If
it turns out that qsort(NULL, 0, ...) is invalid, I will submit a
separate patch to fix it.

BTW, should this patch include a Fixes tag and a Cc @stable?

[1]: https://en.cppreference.com/w/c/algorithm/qsort

Regards,
Kuan-Wei