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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zt4Oc+4/DPqDSsoN@visitorckw-System-Product-Name>
Date: Mon, 9 Sep 2024 04:52:03 +0800
From: Kuan-Wei Chiu <visitorckw@...il.com>
To: Quentin Monnet <qmo@...nel.org>
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
	martin.lau@...ux.dev, eddyz87@...il.com, song@...nel.org,
	yonghong.song@...ux.dev, john.fastabend@...il.com,
	kpsingh@...nel.org, sdf@...ichev.me, haoluo@...gle.com,
	jolsa@...nel.org, jserv@...s.ncku.edu.tw, bpf@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] bpftool: Fix undefined behavior caused by shifting into
 the sign bit

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@...il.com>
> 
> 
> Looks good, thank you.
> 
> Acked-by: Quentin Monnet <qmo@...nel.org>
> 
> 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