bpf: Annotate struct bpf_cand_cache with __counted_by()

Commit Message

Thorsten Blum Aug. 13, 2024, 3:17 p.m. UTC

Add the __counted_by compiler attribute to the flexible array member
cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
CONFIG_FORTIFY_SOURCE.

Increment cnt before adding a new struct to the cands array.

Signed-off-by: Thorsten Blum <[e-mail]>
---
 kernel/bpf/btf.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Alexei Starovoitov Aug. 13, 2024, 4:28 p.m. UTC | #1

On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
>
> Add the __counted_by compiler attribute to the flexible array member
> cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> CONFIG_FORTIFY_SOURCE.
>
> Increment cnt before adding a new struct to the cands array.

why? What happens otherwise?

>
> Signed-off-by: Thorsten Blum <[e-mail]>
> ---
>  kernel/bpf/btf.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 520f49f422fe..42bc70a56fcd 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -7240,7 +7240,7 @@ struct bpf_cand_cache {
>         struct {
>                 const struct btf *btf;
>                 u32 id;
> -       } cands[];
> +       } cands[] __counted_by(cnt);
>  };
>
>  static DEFINE_MUTEX(cand_cache_mutex);
> @@ -8784,9 +8784,9 @@ bpf_core_add_cands(struct bpf_cand_cache *cands, const struct btf *targ_btf,
>                 memcpy(new_cands, cands, sizeof_cands(cands->cnt));
>                 bpf_free_cands(cands);
>                 cands = new_cands;
> -               cands->cands[cands->cnt].btf = targ_btf;
> -               cands->cands[cands->cnt].id = i;
>                 cands->cnt++;
> +               cands->cands[cands->cnt - 1].btf = targ_btf;
> +               cands->cands[cands->cnt - 1].id = i;
>         }
>         return cands;
>  }
> --
> 2.46.0
>

Thorsten Blum Aug. 13, 2024, 5:59 p.m. UTC | #2

On 13. Aug 2024, at 18:28, Alexei Starovoitov <[e-mail]> wrote:
> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
>> 
>> Add the __counted_by compiler attribute to the flexible array member
>> cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>> CONFIG_FORTIFY_SOURCE.
>> 
>> Increment cnt before adding a new struct to the cands array.
> 
> why? What happens otherwise?

If you try to access cands->cands[cands->cnt] without incrementing
cands->cnt first, you're essentially accessing the array out of bounds
which will fail during runtime.

You can read more about it at [1] and [2].

> Signed-off-by: Thorsten Blum <[e-mail]>
>> ---
>> kernel/bpf/btf.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 520f49f422fe..42bc70a56fcd 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -7240,7 +7240,7 @@ struct bpf_cand_cache {
>>        struct {
>>                const struct btf *btf;
>>                u32 id;
>> -       } cands[];
>> +       } cands[] __counted_by(cnt);
>> };
>> 
>> static DEFINE_MUTEX(cand_cache_mutex);
>> @@ -8784,9 +8784,9 @@ bpf_core_add_cands(struct bpf_cand_cache *cands, const struct btf *targ_btf,
>>                memcpy(new_cands, cands, sizeof_cands(cands->cnt));
>>                bpf_free_cands(cands);
>>                cands = new_cands;
>> -               cands->cands[cands->cnt].btf = targ_btf;
>> -               cands->cands[cands->cnt].id = i;
>>                cands->cnt++;
>> +               cands->cands[cands->cnt - 1].btf = targ_btf;
>> +               cands->cands[cands->cnt - 1].id = i;
>>        }
>>        return cands;
>> }
>> --
>> 2.46.0
>> 

[1] https://opensource.googleblog.com/2024/07/bounds-checking-flexible-array-members.html
[2] https://embeddedor.com/blog/2024/06/18/how-to-use-the-new-counted_by-attribute-in-c-and-linux/

Alexei Starovoitov Aug. 13, 2024, 6:57 p.m. UTC | #3

On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <[e-mail]> wrote:
>
> On 13. Aug 2024, at 18:28, Alexei Starovoitov <[e-mail]> wrote:
> > On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
> >>
> >> Add the __counted_by compiler attribute to the flexible array member
> >> cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> >> CONFIG_FORTIFY_SOURCE.
> >>
> >> Increment cnt before adding a new struct to the cands array.
> >
> > why? What happens otherwise?
>
> If you try to access cands->cands[cands->cnt] without incrementing
> cands->cnt first, you're essentially accessing the array out of bounds
> which will fail during runtime.

What kind of error/warn do you see ?
Is it runtime or compile time?

Is this the only place?
what about:
        new_cands = kmemdup(cands, sizeof_cands(cands->cnt), GFP_KERNEL);

cnt field gets copied with other fields.
Can compiler/runtime catch that?

> You can read more about it at [1] and [2].
>
> > Signed-off-by: Thorsten Blum <[e-mail]>
> >> ---
> >> kernel/bpf/btf.c | 6 +++---
> >> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> >> index 520f49f422fe..42bc70a56fcd 100644
> >> --- a/kernel/bpf/btf.c
> >> +++ b/kernel/bpf/btf.c
> >> @@ -7240,7 +7240,7 @@ struct bpf_cand_cache {
> >>        struct {
> >>                const struct btf *btf;
> >>                u32 id;
> >> -       } cands[];
> >> +       } cands[] __counted_by(cnt);
> >> };
> >>
> >> static DEFINE_MUTEX(cand_cache_mutex);
> >> @@ -8784,9 +8784,9 @@ bpf_core_add_cands(struct bpf_cand_cache *cands, const struct btf *targ_btf,
> >>                memcpy(new_cands, cands, sizeof_cands(cands->cnt));
> >>                bpf_free_cands(cands);
> >>                cands = new_cands;
> >> -               cands->cands[cands->cnt].btf = targ_btf;
> >> -               cands->cands[cands->cnt].id = i;
> >>                cands->cnt++;
> >> +               cands->cands[cands->cnt - 1].btf = targ_btf;
> >> +               cands->cands[cands->cnt - 1].id = i;
> >>        }
> >>        return cands;
> >> }
> >> --
> >> 2.46.0
> >>
>
> [1] https://opensource.googleblog.com/2024/07/bounds-checking-flexible-array-members.html
> [2] https://embeddedor.com/blog/2024/06/18/how-to-use-the-new-counted_by-attribute-in-c-and-linux/

Eduard Zingerman Aug. 13, 2024, 8:12 p.m. UTC | #4

On Tue, 2024-08-13 at 11:57 -0700, Alexei Starovoitov wrote:
> On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <[e-mail]> wrote:
> >
> > On 13. Aug 2024, at 18:28, Alexei Starovoitov <[e-mail]> wrote:
> > > On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
> > > >
> > > > Add the __counted_by compiler attribute to the flexible array member
> > > > cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> > > > CONFIG_FORTIFY_SOURCE.
> > > >
> > > > Increment cnt before adding a new struct to the cands array.
> > >
> > > why? What happens otherwise?
> >
> > If you try to access cands->cands[cands->cnt] without incrementing
> > cands->cnt first, you're essentially accessing the array out of bounds
> > which will fail during runtime.
>
> What kind of error/warn do you see ?
> Is it runtime or compile time?
>
> Is this the only place?
> what about:
>         new_cands = kmemdup(cands, sizeof_cands(cands->cnt), GFP_KERNEL);
>
> cnt field gets copied with other fields.
> Can compiler/runtime catch that?

I think that generated check is mechanical, sanitizer wraps access to
array with size check using the value of associated counter, e.g:

    12:52:20 tmp$ clang -fsanitize=undefined ./test.c
    12:52:53 tmp$ ./a.out
    test.c:11:3: runtime error: index 0 out of bounds for type 'int[]'
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test.c:11:3
    12:52:55 tmp$ cat test.c
    #include <alloca.h>

    struct arr {
      int cnt;
      int items[] __attribute__((__counted_by__(cnt)));
    };

    int main(int argc, char **argv) {
      struct arr *arr = alloca(sizeof(struct arr) + sizeof(int));
      arr->cnt = 0;
      arr->items[arr->cnt] = 42;
      arr->cnt++;
      asm volatile (""::"r"(arr));
      return 0;
    }
    12:53:07 tmp$ clang -fsanitize=undefined ./test.c
    12:53:10 tmp$ ./a.out
    test.c:11:3: runtime error: index 0 out of bounds for type 'int[]'
    SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior test.c:11:3
    12:53:13 tmp$ cat test.c
    #include <alloca.h>

    struct arr {
      int cnt;
      int items[] __attribute__((__counted_by__(cnt)));
    };

    int main(int argc, char **argv) {
      struct arr *arr = alloca(sizeof(struct arr) + sizeof(int));
      arr->cnt = 1;
      arr->items[arr->cnt - 1] = 42;
      asm volatile (""::"r"(arr));
      return 0;
    }
    12:53:34 tmp$ clang -fsanitize=undefined ./test.c
    12:53:36 tmp$ ./a.out
    12:53:38 tmp$ echo $?
    0

Or here is the IR generated for C program:

    struct arr {
      unsigned int cnt;
      int items[] __attribute__((__counted_by__(cnt)));
    };

    void push(int i, struct arr *arr) {
      arr->items[arr->cnt] = 42;
      arr->cnt++;
    }

Note the 'cnt' passed as a parameter to '@__ubsan_handle_out_of_bounds':

    define dso_local void @push(i32 noundef %0, ptr noundef %1) local_unnamed_addr #0 !func_sanitize !3 {
      ...
      %11 = load i32, ptr %1, align 4
      %12 = zext i32 %11 to i64
      tail call void @__ubsan_handle_out_of_bounds(ptr nonnull @6, i64 %12) #2, !nosanitize !4

[...]

Thorsten Blum Aug. 13, 2024, 8:51 p.m. UTC | #5

On 13. Aug 2024, at 20:57, Alexei Starovoitov <[e-mail]> wrote:
> On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <[e-mail]> wrote:
>> On 13. Aug 2024, at 18:28, Alexei Starovoitov <[e-mail]> wrote:
>>> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
>>>> 
>>>> Add the __counted_by compiler attribute to the flexible array member
>>>> cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
>>>> CONFIG_FORTIFY_SOURCE.
>>>> 
>>>> Increment cnt before adding a new struct to the cands array.
>>> 
>>> why? What happens otherwise?
>> 
>> If you try to access cands->cands[cands->cnt] without incrementing
>> cands->cnt first, you're essentially accessing the array out of bounds
>> which will fail during runtime.
> 
> What kind of error/warn do you see ?
> Is it runtime or compile time?

I get a runtime error with Clang 18 [3].

> Is this the only place?

I think so.

> what about:
>       new_cands = kmemdup(cands, sizeof_cands(cands->cnt), GFP_KERNEL);
> 
> cnt field gets copied with other fields.
> Can compiler/runtime catch that?

I think this is ok and there's nothing to catch.

> You can read more about it at [1] and [2].
>> 
>>> Signed-off-by: Thorsten Blum <[e-mail]>
>>>> ---
>>>> kernel/bpf/btf.c | 6 +++---
>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>>>> index 520f49f422fe..42bc70a56fcd 100644
>>>> --- a/kernel/bpf/btf.c
>>>> +++ b/kernel/bpf/btf.c
>>>> @@ -7240,7 +7240,7 @@ struct bpf_cand_cache {
>>>>      struct {
>>>>              const struct btf *btf;
>>>>              u32 id;
>>>> -       } cands[];
>>>> +       } cands[] __counted_by(cnt);
>>>> };
>>>> 
>>>> static DEFINE_MUTEX(cand_cache_mutex);
>>>> @@ -8784,9 +8784,9 @@ bpf_core_add_cands(struct bpf_cand_cache *cands, const struct btf *targ_btf,
>>>>              memcpy(new_cands, cands, sizeof_cands(cands->cnt));
>>>>              bpf_free_cands(cands);
>>>>              cands = new_cands;
>>>> -               cands->cands[cands->cnt].btf = targ_btf;
>>>> -               cands->cands[cands->cnt].id = i;
>>>>              cands->cnt++;
>>>> +               cands->cands[cands->cnt - 1].btf = targ_btf;
>>>> +               cands->cands[cands->cnt - 1].id = i;
>>>>      }
>>>>      return cands;
>>>> }
>>>> --
>>>> 2.46.0
>>>> 
>> 
>> [1] https://opensource.googleblog.com/2024/07/bounds-checking-flexible-array-members.html
>> [2] https://embeddedor.com/blog/2024/06/18/how-to-use-the-new-counted_by-attribute-in-c-and-linux/

[3] https://godbolt.org/z/cKee95777

Alexei Starovoitov Aug. 14, 2024, 3:46 p.m. UTC | #6

On Tue, Aug 13, 2024 at 1:51 PM Thorsten Blum <[e-mail]> wrote:
>
> On 13. Aug 2024, at 20:57, Alexei Starovoitov <[e-mail]> wrote:
> > On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <[e-mail]> wrote:
> >> On 13. Aug 2024, at 18:28, Alexei Starovoitov <[e-mail]> wrote:
> >>> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <[e-mail]> wrote:
> >>>>
> >>>> Add the __counted_by compiler attribute to the flexible array member
> >>>> cands to improve access bounds-checking via CONFIG_UBSAN_BOUNDS and
> >>>> CONFIG_FORTIFY_SOURCE.
> >>>>
> >>>> Increment cnt before adding a new struct to the cands array.
> >>>
> >>> why? What happens otherwise?
> >>
> >> If you try to access cands->cands[cands->cnt] without incrementing
> >> cands->cnt first, you're essentially accessing the array out of bounds
> >> which will fail during runtime.
> >
> > What kind of error/warn do you see ?
> > Is it runtime or compile time?
>
> I get a runtime error with Clang 18 [3].

...

> [3] https://godbolt.org/z/cKee95777

This is user space.
I'm not asking about generic description of the counted_by feature.
I want to see the actual runtime report from the kernel.
Can it even compile the kernel with -fsanitize=undefined ?

pw-bot: cr

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 520f49f422fe..42bc70a56fcd 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7240,7 +7240,7 @@  struct bpf_cand_cache {
 	struct {
 		const struct btf *btf;
 		u32 id;
-	} cands[];
+	} cands[] __counted_by(cnt);
 };
 
 static DEFINE_MUTEX(cand_cache_mutex);
@@ -8784,9 +8784,9 @@  bpf_core_add_cands(struct bpf_cand_cache *cands, const struct btf *targ_btf,
 		memcpy(new_cands, cands, sizeof_cands(cands->cnt));
 		bpf_free_cands(cands);
 		cands = new_cands;
-		cands->cands[cands->cnt].btf = targ_btf;
-		cands->cands[cands->cnt].id = i;
 		cands->cnt++;
+		cands->cands[cands->cnt - 1].btf = targ_btf;
+		cands->cands[cands->cnt - 1].id = i;
 	}
 	return cands;
 }