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

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

Posted by Kuan-Wei Chiu 2 months, 3 weeks ago

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@gmail.com>
---
 tools/bpf/bpftool/net.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

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("");
 			}
-- 
2.34.1

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

Posted by Quentin Monnet 2 months, 3 weeks ago

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@gmail.com>


Looks good, thank you.

Acked-by: Quentin Monnet <qmo@kernel.org>

How did you find these?

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

Posted by Kuan-Wei Chiu 2 months, 3 weeks ago

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

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

Posted by Quentin Monnet 2 months, 3 weeks ago

2024-09-09 04:52 UTC+0800 ~ Kuan-Wei Chiu <visitorckw@gmail.com>
> 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@gmail.com>
>>
>>
>> Looks good, thank you.
>>
>> Acked-by: Quentin Monnet <qmo@kernel.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.


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