| Message ID | YtZ+LpgPADm7BeEd@kili (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c6018fc6e7b6fcc4ac5430870c2e8b4ca556621a |
| Delegated to: | BPF |
| Headers | show |
| Series | libbpf: fix sign expansion bug in btf_dump_get_enum_value() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | success | Logs for Kernel LATEST on z15 with gcc |
On Tue, Jul 19, 2022 at 12:49:34PM +0300, Dan Carpenter wrote: > The code here is supposed to take a signed int and store it in a > signed long long. Unfortunately, the way that the type promotion works > with this conditional statement is that it takes a signed int, type > promotes it to a __u32, and then stores that as a signed long long. > The result is never negative. > > Fixes: d90ec262b35b ("libbpf: Add enum64 support for btf_dump") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > tools/lib/bpf/btf_dump.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > index 400e84fd0578..627edb5bb6de 100644 > --- a/tools/lib/bpf/btf_dump.c > +++ b/tools/lib/bpf/btf_dump.c > @@ -2045,7 +2045,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d, > *value = *(__s64 *)data; > return 0; > case 4: > - *value = is_signed ? *(__s32 *)data : *(__u32 *)data; > + *value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data; Only case 4 has issues and what does the standard say ? Do you have a sample dump to debug this that can be pasted in the commit log? > return 0; > case 2: > *value = is_signed ? *(__s16 *)data : *(__u16 *)data; > -- > 2.35.1 >
On Tue, Jul 19, 2022 at 10:26:40AM -0700, Martin KaFai Lau wrote: > On Tue, Jul 19, 2022 at 12:49:34PM +0300, Dan Carpenter wrote: > > The code here is supposed to take a signed int and store it in a > > signed long long. Unfortunately, the way that the type promotion works > > with this conditional statement is that it takes a signed int, type > > promotes it to a __u32, and then stores that as a signed long long. > > The result is never negative. > > > > Fixes: d90ec262b35b ("libbpf: Add enum64 support for btf_dump") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > tools/lib/bpf/btf_dump.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > index 400e84fd0578..627edb5bb6de 100644 > > --- a/tools/lib/bpf/btf_dump.c > > +++ b/tools/lib/bpf/btf_dump.c > > @@ -2045,7 +2045,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d, > > *value = *(__s64 *)data; > > return 0; > > case 4: > > - *value = is_signed ? *(__s32 *)data : *(__u32 *)data; > > + *value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data; > Only case 4 has issues and what does the standard say ? > It looks weird, doesn't it? Yes. Everything smaller than int gets type promoted to int so the sign is extended properly. The only thing larger than s/u32 is s/u64 which is already the right size. > Do you have a sample dump to debug this that can be pasted in the commit log? This is from static analysis, but I made a little test program just to test it before I sent the patch: #include <stdio.h> int main(void) { unsigned long long src = -1ULL; signed long long dst1, dst2; int is_signed = 1; dst1 = is_signed ? *(int *)&src : *(unsigned int *)0; dst2 = is_signed ? (signed long long)*(int *)&src : *(unsigned int *)0; printf("%lld\n", dst1); printf("%lld\n", dst2); return 0; } regards, dan carpenter
On Tue, Jul 19, 2022 at 09:34:13PM +0300, Dan Carpenter wrote: > On Tue, Jul 19, 2022 at 10:26:40AM -0700, Martin KaFai Lau wrote: > > On Tue, Jul 19, 2022 at 12:49:34PM +0300, Dan Carpenter wrote: > > > The code here is supposed to take a signed int and store it in a > > > signed long long. Unfortunately, the way that the type promotion works > > > with this conditional statement is that it takes a signed int, type > > > promotes it to a __u32, and then stores that as a signed long long. > > > The result is never negative. > > > > > > Fixes: d90ec262b35b ("libbpf: Add enum64 support for btf_dump") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > tools/lib/bpf/btf_dump.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c > > > index 400e84fd0578..627edb5bb6de 100644 > > > --- a/tools/lib/bpf/btf_dump.c > > > +++ b/tools/lib/bpf/btf_dump.c > > > @@ -2045,7 +2045,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d, > > > *value = *(__s64 *)data; > > > return 0; > > > case 4: > > > - *value = is_signed ? *(__s32 *)data : *(__u32 *)data; > > > + *value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data; > > Only case 4 has issues and what does the standard say ? > > > > It looks weird, doesn't it? > > Yes. Everything smaller than int gets type promoted to int so the sign > is extended properly. The only thing larger than s/u32 is s/u64 which > is already the right size. Ah. tricky. > > > Do you have a sample dump to debug this that can be pasted in the commit log? > > This is from static analysis, but I made a little test program just to > test it before I sent the patch: > > #include <stdio.h> > > int main(void) > { > unsigned long long src = -1ULL; > signed long long dst1, dst2; > int is_signed = 1; > > dst1 = is_signed ? *(int *)&src : *(unsigned int *)0; > dst2 = is_signed ? (signed long long)*(int *)&src : *(unsigned int *)0; > > printf("%lld\n", dst1); > printf("%lld\n", dst2); > > return 0; > } Thanks for the demo. Acked-by: Martin KaFai Lau <kafai@fb.com>
Hello: This patch was applied to bpf/bpf-next.git (master) by Daniel Borkmann <daniel@iogearbox.net>: On Tue, 19 Jul 2022 12:49:34 +0300 you wrote: > The code here is supposed to take a signed int and store it in a > signed long long. Unfortunately, the way that the type promotion works > with this conditional statement is that it takes a signed int, type > promotes it to a __u32, and then stores that as a signed long long. > The result is never negative. > > Fixes: d90ec262b35b ("libbpf: Add enum64 support for btf_dump") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > [...] Here is the summary with links: - libbpf: fix sign expansion bug in btf_dump_get_enum_value() https://git.kernel.org/bpf/bpf-next/c/c6018fc6e7b6 You are awesome, thank you!
diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c index 400e84fd0578..627edb5bb6de 100644 --- a/tools/lib/bpf/btf_dump.c +++ b/tools/lib/bpf/btf_dump.c @@ -2045,7 +2045,7 @@ static int btf_dump_get_enum_value(struct btf_dump *d, *value = *(__s64 *)data; return 0; case 4: - *value = is_signed ? *(__s32 *)data : *(__u32 *)data; + *value = is_signed ? (__s64)*(__s32 *)data : *(__u32 *)data; return 0; case 2: *value = is_signed ? *(__s16 *)data : *(__u16 *)data;
The code here is supposed to take a signed int and store it in a signed long long. Unfortunately, the way that the type promotion works with this conditional statement is that it takes a signed int, type promotes it to a __u32, and then stores that as a signed long long. The result is never negative. Fixes: d90ec262b35b ("libbpf: Add enum64 support for btf_dump") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- tools/lib/bpf/btf_dump.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)