[PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[Dan Carpenter]

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;
 		return 0;
 	case 2:
 		*value = is_signed ? *(__s16 *)data : *(__u16 *)data;
-- 
2.35.1

Re: [PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[Martin KaFai Lau]

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
> 

Re: [PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[Dan Carpenter]

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

Re: [PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[Martin KaFai Lau]

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>

Re: [PATCH] libbpf: fix sign expansion bug in btf_dump_get_enum_value()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html