All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] libbpf: Check full backing type size for bitfield dump
@ 2026-07-05  3:26 Guangshuo Li
  2026-07-05  3:38 ` sashiko-bot
  2026-07-05 22:39 ` Varun R Mallya
  0 siblings, 2 replies; 4+ messages in thread
From: Guangshuo Li @ 2026-07-05  3:26 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
	Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis,
	Varun R Mallya, bpf, linux-kernel
  Cc: Guangshuo Li

btf_dump_get_bitfield_value() builds a bitfield value by reading bytes
from the backing integer type. The bounds check added for short data
buffers uses the number of bytes covered by the bitfield itself, but the
read loop consumes the full backing type width.

For narrow bitfields this can leave part of the backing type unchecked.
For example, a one-bit field backed by a four-byte integer only requires
one byte according to the bitfield span calculation, while the value
construction still reads four bytes.

Check the backing type size instead, so the bounds check matches the
actual read range.

Fixes: 5714ca8cba5e ("libbpf: Fix OOB read in btf_dump_get_bitfield_value")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.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 cc1ba65bb6c5..a49790e356a8 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -1771,7 +1771,7 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
 	nr_bytes = (start_bit + bit_sz + 7) / 8;
 
 	/* Bound check */
-	if (data + nr_bytes > d->typed_dump->data_end)
+	if (data + t->size > d->typed_dump->data_end)
 		return -E2BIG;
 
 	/* Maximum supported bitfield size is 64 bits */
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] libbpf: Check full backing type size for bitfield dump
  2026-07-05  3:26 [PATCH] libbpf: Check full backing type size for bitfield dump Guangshuo Li
@ 2026-07-05  3:38 ` sashiko-bot
  2026-07-05 22:39 ` Varun R Mallya
  1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-05  3:38 UTC (permalink / raw)
  To: Guangshuo Li; +Cc: bpf

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The stricter bounds check using `t->size` incorrectly rejects valid narrow bitfields in packed structs, causing a false positive `-E2BIG` failure.
- [Medium] The bounds check for bitfields was updated in `btf_dump_get_bitfield_value` to use `t->size`, but the early bounds check in `btf_dump_type_data_check_overflow` was left unpatched, causing inconsistent behavior.
- [Low] Unused local variables `start_bit` and `nr_bytes` are left behind after the patch.
--

commit dcf1121c5988139c35dcd52cf31ce52003f38d43
Author: Guangshuo Li <lgs201920130244@gmail.com>

libbpf: Check full backing type size for bitfield dump

This patch updates the bounds check in btf_dump_get_bitfield_value() to use
the full backing type size instead of just the bitfield span. This prevents
out-of-bounds reads when constructing bitfield values from narrow data
buffers.

> diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
> index cc1ba65bb6c5a..a49790e356a83 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1771,7 +1771,7 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
>  	nr_bytes = (start_bit + bit_sz + 7) / 8;

[Severity: Low]
Are start_bit and nr_bytes now unused variables? Since the bounds check
below was updated to use t->size instead of nr_bytes, it looks like these
calculations are dead code and might trigger compiler warnings.

>  
>  	/* Bound check */
> -	if (data + nr_bytes > d->typed_dump->data_end)
> +	if (data + t->size > d->typed_dump->data_end)

[Severity: High]
Will this stricter bounds check reject valid narrow bitfields in packed
structs? If a user dumps a 1-byte packed struct containing an int
bitfield (e.g., int a:8), the backing type size (t->size) will be 4, but
the valid data size is only 1 byte. Could this cause the libbpf BTF dump
API to fail with -E2BIG for valid structs?

[Severity: Medium]
Does this change create an inconsistency with the early bounds check in
btf_dump_type_data_check_overflow()?

Looking at btf_dump_type_data_check_overflow() in the same file:

	if (btf_is_bitfield(t)) {
		...
		return data + nr_bytes > d->typed_dump->data_end ? -E2BIG : nr_bytes;
	}

Since that function still uses nr_bytes, could it succeed while this
function fails, resulting in partial and malformed output where the dump
prints the struct member prefix but fails during value retrieval?

>  		return -E2BIG;
>  
>  	/* Maximum supported bitfield size is 64 bits */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705032603.275766-1-lgs201920130244@gmail.com?part=1

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] libbpf: Check full backing type size for bitfield dump
  2026-07-05  3:26 [PATCH] libbpf: Check full backing type size for bitfield dump Guangshuo Li
  2026-07-05  3:38 ` sashiko-bot
@ 2026-07-05 22:39 ` Varun R Mallya
  2026-07-06  2:29   ` Guangshuo Li
  1 sibling, 1 reply; 4+ messages in thread
From: Varun R Mallya @ 2026-07-05 22:39 UTC (permalink / raw)
  To: Guangshuo Li
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
	Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis, bpf,
	linux-kernel

On Sun, Jul 05, 2026 at 11:26:03AM +0800, Guangshuo Li wrote:
> btf_dump_get_bitfield_value() builds a bitfield value by reading bytes
> from the backing integer type. The bounds check added for short data
> buffers uses the number of bytes covered by the bitfield itself, but the
> read loop consumes the full backing type width.
> 
> For narrow bitfields this can leave part of the backing type unchecked.
> For example, a one-bit field backed by a four-byte integer only requires
> one byte according to the bitfield span calculation, while the value
> construction still reads four bytes.
> 
> Check the backing type size instead, so the bounds check matches the
> actual read range.
> 
> Fixes: 5714ca8cba5e ("libbpf: Fix OOB read in btf_dump_get_bitfield_value")
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.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 cc1ba65bb6c5..a49790e356a8 100644
> --- a/tools/lib/bpf/btf_dump.c
> +++ b/tools/lib/bpf/btf_dump.c
> @@ -1771,7 +1771,7 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
>  	nr_bytes = (start_bit + bit_sz + 7) / 8;

nr_bytes and the line before it are made obsolete due to your change and
become an unused variable, so that would need cleanup, as sashiko
correctly pointed out.

>  	/* Bound check */
> -	if (data + nr_bytes > d->typed_dump->data_end)
> +	if (data + t->size > d->typed_dump->data_end)

The fix does not seem right to me. This will reject
something like:

  struct packed_int8 {
        int a:8;
  } __attribute__((packed));

with BTF output like:

  STRUCT 'packed_int8' size=1
    'a' type_id=2 bits_offset=0 bitfield_size=8
  INT 'int' size=4

As you can see, this valid object has a size of 1 but BTF type has size
4.
nr_bytes (the old one), would get calculated as 1, and t->size would
show up as 4. This will unncessarily reject valid data.

I do understand the concern with the loops below using t->size but the
check happening on a value calculated separately, and this can actually
trigger an OOB. You could maybe try fixing the loops instead by handling
packed bitfields (like the case I gave) specially, but making the check
more strict is something I don't think is helpful.

>  		return -E2BIG;
>  
>  	/* Maximum supported bitfield size is 64 bits */
> -- 
> 2.43.0
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] libbpf: Check full backing type size for bitfield dump
  2026-07-05 22:39 ` Varun R Mallya
@ 2026-07-06  2:29   ` Guangshuo Li
  0 siblings, 0 replies; 4+ messages in thread
From: Guangshuo Li @ 2026-07-06  2:29 UTC (permalink / raw)
  To: Varun R Mallya
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Eduard Zingerman, Kumar Kartikeya Dwivedi, Martin KaFai Lau,
	Song Liu, Yonghong Song, Jiri Olsa, Emil Tsalapatis, bpf,
	linux-kernel

Hi Varun,

Thanks for the review.

On Mon, 6 Jul 2026 at 06:39, Varun R Mallya <varunrmallya@gmail.com> wrote:
>
> On Sun, Jul 05, 2026 at 11:26:03AM +0800, Guangshuo Li wrote:
> > btf_dump_get_bitfield_value() builds a bitfield value by reading bytes
> > from the backing integer type. The bounds check added for short data
> > buffers uses the number of bytes covered by the bitfield itself, but the
> > read loop consumes the full backing type width.
> >
> > For narrow bitfields this can leave part of the backing type unchecked.
> > For example, a one-bit field backed by a four-byte integer only requires
> > one byte according to the bitfield span calculation, while the value
> > construction still reads four bytes.
> >
> > Check the backing type size instead, so the bounds check matches the
> > actual read range.
> >
> > Fixes: 5714ca8cba5e ("libbpf: Fix OOB read in btf_dump_get_bitfield_value")
> > Signed-off-by: Guangshuo Li <lgs201920130244@gmail.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 cc1ba65bb6c5..a49790e356a8 100644
> > --- a/tools/lib/bpf/btf_dump.c
> > +++ b/tools/lib/bpf/btf_dump.c
> > @@ -1771,7 +1771,7 @@ static int btf_dump_get_bitfield_value(struct btf_dump *d,
> >       nr_bytes = (start_bit + bit_sz + 7) / 8;
>
> nr_bytes and the line before it are made obsolete due to your change and
> become an unused variable, so that would need cleanup, as sashiko
> correctly pointed out.
>
> >       /* Bound check */
> > -     if (data + nr_bytes > d->typed_dump->data_end)
> > +     if (data + t->size > d->typed_dump->data_end)
>
> The fix does not seem right to me. This will reject
> something like:
>
>   struct packed_int8 {
>         int a:8;
>   } __attribute__((packed));
>
> with BTF output like:
>
>   STRUCT 'packed_int8' size=1
>     'a' type_id=2 bits_offset=0 bitfield_size=8
>   INT 'int' size=4
>
> As you can see, this valid object has a size of 1 but BTF type has size
> 4.
> nr_bytes (the old one), would get calculated as 1, and t->size would
> show up as 4. This will unncessarily reject valid data.
>
> I do understand the concern with the loops below using t->size but the
> check happening on a value calculated separately, and this can actually
> trigger an OOB. You could maybe try fixing the loops instead by handling
> packed bitfields (like the case I gave) specially, but making the check
> more strict is something I don't think is helpful.
>
> >               return -E2BIG;
> >
> >       /* Maximum supported bitfield size is 64 bits */
> > --
> > 2.43.0
> >

You are right, checking t->size is too strict for
packed bitfields where the backing integer type can be larger than the
actual object bytes covering the field.

I'll send a v2 that keeps the nr_bytes bounds check and instead changes
the value-building loops to consume only the checked byte range.

Thanks,
Guangshuo

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-07-06  2:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-05  3:26 [PATCH] libbpf: Check full backing type size for bitfield dump Guangshuo Li
2026-07-05  3:38 ` sashiko-bot
2026-07-05 22:39 ` Varun R Mallya
2026-07-06  2:29   ` Guangshuo Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.