bpftool: Fix undefined behavior caused by shifting into the sign bit

Commit Message

Kuan-Wei Chiu Sept. 8, 2024, 2 p.m. UTC

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 <[e-mail]>
---
 tools/bpf/bpftool/net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Quentin Monnet Sept. 8, 2024, 7:48 p.m. UTC | #1

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 <[e-mail]>


Looks good, thank you.

Acked-by: Quentin Monnet <[e-mail]>

How did you find these?

Kuan-Wei Chiu Sept. 8, 2024, 8:52 p.m. UTC | #2

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 <[e-mail]>
> 
> 
> Looks good, thank you.
> 
> Acked-by: Quentin Monnet <[e-mail]>
> 
> 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

[e-mail] Sept. 9, 2024, 11 p.m. UTC | #3

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <[e-mail]>:

On Sun,  8 Sep 2024 22:00:09 +0800 you 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 <[e-mail]>
> 
> [...]

Here is the summary with links:
  - bpftool: Fix undefined behavior caused by shifting into the sign bit
    https://git.kernel.org/bpf/bpf-next/c/4cdc0e4ce5e8

You are awesome, thank you!

Quentin Monnet Sept. 10, 2024, 9:21 a.m. UTC | #4

2024-09-09 04:52 UTC+0800 ~ Kuan-Wei Chiu <[e-mail]>
> 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 <[e-mail]>
>>
>>
>> Looks good, thank you.
>>
>> Acked-by: Quentin Monnet <[e-mail]>
>>
>> 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.


Nice


> 
> 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.


OK, thanks for looking into it!


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

We could maybe have used a Fixes:, but the patch is merged already so 
never mind. As for stable, I don't think this is necessary. I don't 
believe we can hit the undefined behaviour today; and we encourage 
people to package bpftool from the GitHub mirror anyway, where your 
patch will land soon.

Thanks,
Quentin

Patch

diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c
index 968714b4c3d4..ad2ea6cf2db1 100644
--- a/tools/bpf/bpftool/net.c
+++ b/tools/bpf/bpftool/net.c
@@ -482,9 +482,9 @@  static void __show_dev_tc_bpf(const struct ip_devname_ifindex *dev,
 		if (prog_flags[i] || json_output) {
 			NET_START_ARRAY("prog_flags", "%s ");
 			for (j = 0; prog_flags[i] && j < 32; j++) {
-				if (!(prog_flags[i] & (1 << j)))
+				if (!(prog_flags[i] & (1U << j)))
 					continue;
-				NET_DUMP_UINT_ONLY(1 << j);
+				NET_DUMP_UINT_ONLY(1U << j);
 			}
 			NET_END_ARRAY("");
 		}
@@ -493,9 +493,9 @@  static void __show_dev_tc_bpf(const struct ip_devname_ifindex *dev,
 			if (link_flags[i] || json_output) {
 				NET_START_ARRAY("link_flags", "%s ");
 				for (j = 0; link_flags[i] && j < 32; j++) {
-					if (!(link_flags[i] & (1 << j)))
+					if (!(link_flags[i] & (1U << j)))
 						continue;
-					NET_DUMP_UINT_ONLY(1 << j);
+					NET_DUMP_UINT_ONLY(1U << j);
 				}
 				NET_END_ARRAY("");
 			}