| Message ID | 20240813151752.95161-2-thorsten.blum@toblux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bpf: Annotate struct bpf_cand_cache with __counted_by() | expand |
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? > > 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 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 [...]
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
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; }
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(-)