kernel/bpf/btf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
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 <thorsten.blum@toblux.com>
---
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.0On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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 <thorsten.blum@toblux.com>
> ---
> 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
>
On 13. Aug 2024, at 18:28, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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 <thorsten.blum@toblux.com>
>> ---
>> 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/
On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>
> On 13. Aug 2024, at 18:28, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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 <thorsten.blum@toblux.com>
> >> ---
> >> 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/
On 13. Aug 2024, at 20:57, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
>> On 13. Aug 2024, at 18:28, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
>>> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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 <thorsten.blum@toblux.com>
>>>> ---
>>>> 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
On Tue, Aug 13, 2024 at 1:51 PM Thorsten Blum <thorsten.blum@toblux.com> wrote: > > On 13. Aug 2024, at 20:57, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <thorsten.blum@toblux.com> wrote: > >> On 13. Aug 2024, at 18:28, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > >>> On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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
On Tue, 2024-08-13 at 11:57 -0700, Alexei Starovoitov wrote:
> On Tue, Aug 13, 2024 at 10:59 AM Thorsten Blum <thorsten.blum@toblux.com> wrote:
> >
> > On 13. Aug 2024, at 18:28, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > > On Tue, Aug 13, 2024 at 8:19 AM Thorsten Blum <thorsten.blum@toblux.com> 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
[...]
© 2016 - 2026 Red Hat, Inc.