* [PATCH] comedi: Annotate struct comedi_lrange with __counted_by @ 2023-09-30 9:14 Christophe JAILLET 2023-09-30 20:57 ` Kees Cook 2023-10-01 6:43 ` Gustavo A. R. Silva 0 siblings, 2 replies; 14+ messages in thread From: Christophe JAILLET @ 2023-09-30 9:14 UTC (permalink / raw) To: Ian Abbott, H Hartley Sweeten, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-hardening, llvm Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- This patch is part of a work done in parallel of what is currently worked on by Kees Cook. My patches are only related to corner cases that do NOT match the semantic of his Coccinelle script[1]. In this case, it is been spotted because of comedi_alloc_spriv(). All other usages of struct comedi_lrange seem to be static definition of the structure that explicitly set the .length field. [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci --- include/linux/comedi/comedidev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h index 0a1150900ef3..c08416a7364b 100644 --- a/include/linux/comedi/comedidev.h +++ b/include/linux/comedi/comedidev.h @@ -633,7 +633,7 @@ extern const struct comedi_lrange range_unknown; */ struct comedi_lrange { int length; - struct comedi_krange range[]; + struct comedi_krange range[] __counted_by(length); }; /** -- 2.34.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-09-30 9:14 [PATCH] comedi: Annotate struct comedi_lrange with __counted_by Christophe JAILLET @ 2023-09-30 20:57 ` Kees Cook 2023-10-01 7:25 ` Julia Lawall 2023-10-01 7:45 ` Julia Lawall 2023-10-01 6:43 ` Gustavo A. R. Silva 1 sibling, 2 replies; 14+ messages in thread From: Kees Cook @ 2023-09-30 20:57 UTC (permalink / raw) To: Christophe JAILLET Cc: Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sat, Sep 30, 2023 at 11:14:47AM +0200, Christophe JAILLET wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > This patch is part of a work done in parallel of what is currently worked > on by Kees Cook. > > My patches are only related to corner cases that do NOT match the > semantic of his Coccinelle script[1]. Nice! struct comedi_lrange { int length; struct comedi_krange range[]; }; ... static const struct comedi_lrange range_rti800_ai_10_bipolar = { 4, { BIP_RANGE(10), BIP_RANGE(1), BIP_RANGE(0.1), BIP_RANGE(0.02) } }; I'm struggling to come up with a way for Coccinelle to find this kind of thing in other places... > In this case, it is been spotted because of comedi_alloc_spriv(). > All other usages of struct comedi_lrange seem to be static definition of > the structure that explicitly set the .length field. Ah-ha, I found it in drivers/comedi/drivers/das16.c das16_ai_range(): lrange = comedi_alloc_spriv(s, struct_size(lrange, range, 1)); I was also able to find this: union jr3_pci_single_range { struct comedi_lrange l; char _reserved[offsetof(struct comedi_lrange, range[1])]; }; Which looks a lot like DEFINE_FLEX: https://lore.kernel.org/linux-hardening/20230912115937.1645707-2-przemyslaw.kitszel@intel.com/ But that above for stack varaibles rather than globals. But I'm way off topic now. ;) Reviewed-by: Kees Cook <keescook@chromium.org> > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > --- > include/linux/comedi/comedidev.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h > index 0a1150900ef3..c08416a7364b 100644 > --- a/include/linux/comedi/comedidev.h > +++ b/include/linux/comedi/comedidev.h > @@ -633,7 +633,7 @@ extern const struct comedi_lrange range_unknown; > */ > struct comedi_lrange { > int length; > - struct comedi_krange range[]; > + struct comedi_krange range[] __counted_by(length); > }; > > /** > -- > 2.34.1 > -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-09-30 20:57 ` Kees Cook @ 2023-10-01 7:25 ` Julia Lawall 2023-10-01 7:50 ` Christophe JAILLET 2023-10-01 7:45 ` Julia Lawall 1 sibling, 1 reply; 14+ messages in thread From: Julia Lawall @ 2023-10-01 7:25 UTC (permalink / raw) To: Kees Cook Cc: Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sat, 30 Sep 2023, Kees Cook wrote: > On Sat, Sep 30, 2023 at 11:14:47AM +0200, Christophe JAILLET wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > This patch is part of a work done in parallel of what is currently worked > > on by Kees Cook. > > > > My patches are only related to corner cases that do NOT match the > > semantic of his Coccinelle script[1]. > > Nice! > > struct comedi_lrange { > int length; > struct comedi_krange range[]; > }; > ... > static const struct comedi_lrange range_rti800_ai_10_bipolar = { > 4, { > BIP_RANGE(10), > BIP_RANGE(1), > BIP_RANGE(0.1), > BIP_RANGE(0.02) > } > }; > > I'm struggling to come up with a way for Coccinelle to find this kind of > thing in other places... > > > In this case, it is been spotted because of comedi_alloc_spriv(). > > All other usages of struct comedi_lrange seem to be static definition of > > the structure that explicitly set the .length field. > > Ah-ha, I found it in drivers/comedi/drivers/das16.c das16_ai_range(): > > lrange = comedi_alloc_spriv(s, > struct_size(lrange, range, 1)); This is not found due to the regular expression used for the name of the alloc function. Maybe you could drop it entirely? Maybe you could just check for alloc somewhere in the string? identifier ALLOC =~ "alloc"; works in this case. Also, I see in the link that you have: // Options: --all-includes You can actually force this by putting #spatch --all-includes and any other options you want. julia > > I was also able to find this: > > union jr3_pci_single_range { > struct comedi_lrange l; > char _reserved[offsetof(struct comedi_lrange, range[1])]; > }; > > Which looks a lot like DEFINE_FLEX: > https://lore.kernel.org/linux-hardening/20230912115937.1645707-2-przemyslaw.kitszel@intel.com/ > But that above for stack varaibles rather than globals. But I'm way off > topic now. ;) > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > --- > > include/linux/comedi/comedidev.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h > > index 0a1150900ef3..c08416a7364b 100644 > > --- a/include/linux/comedi/comedidev.h > > +++ b/include/linux/comedi/comedidev.h > > @@ -633,7 +633,7 @@ extern const struct comedi_lrange range_unknown; > > */ > > struct comedi_lrange { > > int length; > > - struct comedi_krange range[]; > > + struct comedi_krange range[] __counted_by(length); > > }; > > > > /** > > -- > > 2.34.1 > > > > -- > Kees Cook > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 7:25 ` Julia Lawall @ 2023-10-01 7:50 ` Christophe JAILLET 0 siblings, 0 replies; 14+ messages in thread From: Christophe JAILLET @ 2023-10-01 7:50 UTC (permalink / raw) To: Julia Lawall, Kees Cook Cc: Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm Le 01/10/2023 à 09:25, Julia Lawall a écrit : > > This is not found due to the regular expression used for the name of the > alloc function. Maybe you could drop it entirely? Maybe you could just > check for alloc somewhere in the string? That's how I found it. I simplified a lot Kees's script and looked for function names that did *not* match his regex. Functions that: - return a pointer to a struct - are used with struct_size() - store the value used to compite the size in another field of the struct are good enough candidates. I think that removing the regex all together would be just good enough. CJ > > identifier ALLOC =~ "alloc"; > > works in this case. > > Also, I see in the link that you have: > > // Options: --all-includes > > You can actually force this by putting > > #spatch --all-includes Nice, thanks for the tip. > > and any other options you want. > > julia > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-09-30 20:57 ` Kees Cook 2023-10-01 7:25 ` Julia Lawall @ 2023-10-01 7:45 ` Julia Lawall 2023-10-01 15:26 ` Kees Cook 1 sibling, 1 reply; 14+ messages in thread From: Julia Lawall @ 2023-10-01 7:45 UTC (permalink / raw) To: Kees Cook Cc: Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sat, 30 Sep 2023, Kees Cook wrote: > On Sat, Sep 30, 2023 at 11:14:47AM +0200, Christophe JAILLET wrote: > > Prepare for the coming implementation by GCC and Clang of the __counted_by > > attribute. Flexible array members annotated with __counted_by can have > > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > > functions). > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > This patch is part of a work done in parallel of what is currently worked > > on by Kees Cook. > > > > My patches are only related to corner cases that do NOT match the > > semantic of his Coccinelle script[1]. > > Nice! > > struct comedi_lrange { > int length; > struct comedi_krange range[]; > }; > ... > static const struct comedi_lrange range_rti800_ai_10_bipolar = { > 4, { > BIP_RANGE(10), > BIP_RANGE(1), > BIP_RANGE(0.1), > BIP_RANGE(0.02) > } > }; > > I'm struggling to come up with a way for Coccinelle to find this kind of > thing in other places... Kees, what exactly are you trying to match? Static allocations? julia > > > In this case, it is been spotted because of comedi_alloc_spriv(). > > All other usages of struct comedi_lrange seem to be static definition of > > the structure that explicitly set the .length field. > > Ah-ha, I found it in drivers/comedi/drivers/das16.c das16_ai_range(): > > lrange = comedi_alloc_spriv(s, > struct_size(lrange, range, 1)); > > I was also able to find this: > > union jr3_pci_single_range { > struct comedi_lrange l; > char _reserved[offsetof(struct comedi_lrange, range[1])]; > }; > > Which looks a lot like DEFINE_FLEX: > https://lore.kernel.org/linux-hardening/20230912115937.1645707-2-przemyslaw.kitszel@intel.com/ > But that above for stack varaibles rather than globals. But I'm way off > topic now. ;) > > Reviewed-by: Kees Cook <keescook@chromium.org> > > > > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > > --- > > include/linux/comedi/comedidev.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h > > index 0a1150900ef3..c08416a7364b 100644 > > --- a/include/linux/comedi/comedidev.h > > +++ b/include/linux/comedi/comedidev.h > > @@ -633,7 +633,7 @@ extern const struct comedi_lrange range_unknown; > > */ > > struct comedi_lrange { > > int length; > > - struct comedi_krange range[]; > > + struct comedi_krange range[] __counted_by(length); > > }; > > > > /** > > -- > > 2.34.1 > > > > -- > Kees Cook > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 7:45 ` Julia Lawall @ 2023-10-01 15:26 ` Kees Cook 2023-10-01 19:14 ` Julia Lawall 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2023-10-01 15:26 UTC (permalink / raw) To: Julia Lawall, Kees Cook Cc: Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On October 1, 2023 12:45:41 AM PDT, Julia Lawall <julia.lawall@inria.fr> wrote: > > >On Sat, 30 Sep 2023, Kees Cook wrote: > >> On Sat, Sep 30, 2023 at 11:14:47AM +0200, Christophe JAILLET wrote: >> > Prepare for the coming implementation by GCC and Clang of the __counted_by >> > attribute. Flexible array members annotated with __counted_by can have >> > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS >> > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family >> > functions). >> > >> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> > --- >> > This patch is part of a work done in parallel of what is currently worked >> > on by Kees Cook. >> > >> > My patches are only related to corner cases that do NOT match the >> > semantic of his Coccinelle script[1]. >> >> Nice! >> >> struct comedi_lrange { >> int length; >> struct comedi_krange range[]; >> }; >> ... >> static const struct comedi_lrange range_rti800_ai_10_bipolar = { >> 4, { >> BIP_RANGE(10), >> BIP_RANGE(1), >> BIP_RANGE(0.1), >> BIP_RANGE(0.02) >> } >> }; >> >> I'm struggling to come up with a way for Coccinelle to find this kind of >> thing in other places... > >Kees, what exactly are you trying to match? Static allocations? I need to count the number of initialized elements in the flexible array that is the last member and see if it matches a value set in another member. E.g. the above sets 4 values for the last array member and then sets another member to 4. -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 15:26 ` Kees Cook @ 2023-10-01 19:14 ` Julia Lawall 2023-10-01 21:05 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Julia Lawall @ 2023-10-01 19:14 UTC (permalink / raw) To: Kees Cook Cc: Julia Lawall, Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm Kees, You can try the following. julia #spatch --all-includes @r@ identifier i,j; type T; @@ struct i { ... T j[]; } @s@ identifier r.i; constant ini; identifier j; initializer list [n] is2; position p; identifier x; @@ struct i@p x = { ..., .j = ini, ..., { is2 } } ; @script:ocaml@ ini << s.ini; i << r.i; j << s.j; n << s.n; p << s.p; @@ try let ini = int_of_string ini in if n = ini then Printf.printf "%s:%d: struct %s: field %s is the counter for the flex array\n" (List.hd p).file (List.hd p).line i j with _-> () (* not an explicit integer *) @s2@ identifier r.i; constant ini; initializer list [n] is; initializer list [n2] is2; position p; identifier x; @@ struct i@p x = { is, ini, ..., { is2 } }; @script:ocaml@ ini << s2.ini; i << r.i; n << s2.n; n2 << s2.n2; p << s2.p; @@ try let ini = int_of_string ini in if n2 = ini then Printf.printf "%s:%d: struct %s: field at offset %d is the counter for the flex array\n" (List.hd p).file (List.hd p).line i n with _-> () (* not an explicit integer *) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 19:14 ` Julia Lawall @ 2023-10-01 21:05 ` Kees Cook 2023-10-01 21:22 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Kees Cook @ 2023-10-01 21:05 UTC (permalink / raw) To: Julia Lawall Cc: Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sun, Oct 01, 2023 at 09:14:02PM +0200, Julia Lawall wrote: > Kees, > > You can try the following. Cool! Yeah, this finds the example: drivers/comedi/drivers/rti800.c:74: struct comedi_lrange: field at offset 0 is the counter for the flex array drivers/comedi/drivers/rti800.c:83: struct comedi_lrange: field at offset 0 is the counter for the flex array drivers/comedi/drivers/rti800.c:92: struct comedi_lrange: field at offset 0 is the counter for the flex array I'll run it on the whole codebase... -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 21:05 ` Kees Cook @ 2023-10-01 21:22 ` Kees Cook 2023-10-01 22:21 ` Kees Cook 2023-11-05 21:27 ` Christophe JAILLET 0 siblings, 2 replies; 14+ messages in thread From: Kees Cook @ 2023-10-01 21:22 UTC (permalink / raw) To: Julia Lawall Cc: Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sun, Oct 01, 2023 at 02:05:46PM -0700, Kees Cook wrote: > On Sun, Oct 01, 2023 at 09:14:02PM +0200, Julia Lawall wrote: > > Kees, > > > > You can try the following. > > Cool! Yeah, this finds the example: > > drivers/comedi/drivers/rti800.c:74: struct comedi_lrange: field at offset 0 is the counter for the flex array > drivers/comedi/drivers/rti800.c:83: struct comedi_lrange: field at offset 0 is the counter for the flex array > drivers/comedi/drivers/rti800.c:92: struct comedi_lrange: field at offset 0 is the counter for the flex array > > I'll run it on the whole codebase... It found only the struct comedi_lrange instances, but that's good to know. :) -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 21:22 ` Kees Cook @ 2023-10-01 22:21 ` Kees Cook 2023-10-02 5:38 ` Julia Lawall 2023-11-05 21:27 ` Christophe JAILLET 1 sibling, 1 reply; 14+ messages in thread From: Kees Cook @ 2023-10-01 22:21 UTC (permalink / raw) To: Julia Lawall Cc: Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sun, Oct 01, 2023 at 02:22:17PM -0700, Kees Cook wrote: > On Sun, Oct 01, 2023 at 02:05:46PM -0700, Kees Cook wrote: > > On Sun, Oct 01, 2023 at 09:14:02PM +0200, Julia Lawall wrote: > > > Kees, > > > > > > You can try the following. > > > > Cool! Yeah, this finds the example: > > > > drivers/comedi/drivers/rti800.c:74: struct comedi_lrange: field at offset 0 is the counter for the flex array > > drivers/comedi/drivers/rti800.c:83: struct comedi_lrange: field at offset 0 is the counter for the flex array > > drivers/comedi/drivers/rti800.c:92: struct comedi_lrange: field at offset 0 is the counter for the flex array > > > > I'll run it on the whole codebase... > > It found only the struct comedi_lrange instances, but that's good to > know. :) On a related note, why doesn't this work? @allocated@ identifier STRUCT, ARRAY; expression COUNT; struct STRUCT *PTR; identifier ALLOC; type ELEMENT_TYPE; @@ PTR = ALLOC(..., sizeof(\(*PTR\|struct STRUCT\)) + COUNT * sizeof(\(*PTR->ARRAY\|PTR->ARRAY[0]\|ELEMENT_TYPE\)), ...); minus: parse error: File "alloc.cocci", line 15, column 34, charpos = 485 around = 'struct', whole content = PTR = ALLOC(..., sizeof(\(*PTR\|struct STRUCT\)) + if I drop "struct", then it complains about ELEMENT_TYPE... -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 22:21 ` Kees Cook @ 2023-10-02 5:38 ` Julia Lawall 2023-10-02 7:36 ` Kees Cook 0 siblings, 1 reply; 14+ messages in thread From: Julia Lawall @ 2023-10-02 5:38 UTC (permalink / raw) To: Kees Cook Cc: Julia Lawall, Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Sun, 1 Oct 2023, Kees Cook wrote: > On Sun, Oct 01, 2023 at 02:22:17PM -0700, Kees Cook wrote: > > On Sun, Oct 01, 2023 at 02:05:46PM -0700, Kees Cook wrote: > > > On Sun, Oct 01, 2023 at 09:14:02PM +0200, Julia Lawall wrote: > > > > Kees, > > > > > > > > You can try the following. > > > > > > Cool! Yeah, this finds the example: > > > > > > drivers/comedi/drivers/rti800.c:74: struct comedi_lrange: field at offset 0 is the counter for the flex array > > > drivers/comedi/drivers/rti800.c:83: struct comedi_lrange: field at offset 0 is the counter for the flex array > > > drivers/comedi/drivers/rti800.c:92: struct comedi_lrange: field at offset 0 is the counter for the flex array > > > > > > I'll run it on the whole codebase... > > > > It found only the struct comedi_lrange instances, but that's good to > > know. :) > > On a related note, why doesn't this work? > > @allocated@ > identifier STRUCT, ARRAY; > expression COUNT; > struct STRUCT *PTR; > identifier ALLOC; > type ELEMENT_TYPE; > @@ > > PTR = ALLOC(..., sizeof(\(*PTR\|struct STRUCT\)) + > COUNT * sizeof(\(*PTR->ARRAY\|PTR->ARRAY[0]\|ELEMENT_TYPE\)), ...); > > > minus: parse error: > File "alloc.cocci", line 15, column 34, charpos = 485 > around = 'struct', > whole content = PTR = ALLOC(..., sizeof(\(*PTR\|struct STRUCT\)) + > > if I drop "struct", then it complains about ELEMENT_TYPE... The sizeof with an expression argument is treated differently than the sizeof with a type argument. So you need to write: @allocated@ identifier STRUCT, ARRAY; expression COUNT; struct STRUCT *PTR; identifier ALLOC; type ELEMENT_TYPE; @@ PTR = ALLOC(..., \(sizeof(*PTR)\|sizeof(struct STRUCT)\) + COUNT * \(sizeof(*PTR->ARRAY)\|sizeof(PTR->ARRAY[0])\|sizeof(ELEMENT_TYPE)\), ...); julia ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-02 5:38 ` Julia Lawall @ 2023-10-02 7:36 ` Kees Cook 0 siblings, 0 replies; 14+ messages in thread From: Kees Cook @ 2023-10-02 7:36 UTC (permalink / raw) To: Julia Lawall Cc: Kees Cook, Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix, linux-kernel, kernel-janitors, linux-hardening, llvm On Mon, Oct 02, 2023 at 07:38:42AM +0200, Julia Lawall wrote: > The sizeof with an expression argument is treated differently than the > sizeof with a type argument. So you need to write: > > @allocated@ > identifier STRUCT, ARRAY; > expression COUNT; > struct STRUCT *PTR; > identifier ALLOC; > type ELEMENT_TYPE; > @@ > > PTR = ALLOC(..., \(sizeof(*PTR)\|sizeof(struct STRUCT)\) + > COUNT * \(sizeof(*PTR->ARRAY)\|sizeof(PTR->ARRAY[0])\|sizeof(ELEMENT_TYPE)\), ...); Ah! Thank you thank you! Yes, this works great now. :) -- Kees Cook ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-10-01 21:22 ` Kees Cook 2023-10-01 22:21 ` Kees Cook @ 2023-11-05 21:27 ` Christophe JAILLET 1 sibling, 0 replies; 14+ messages in thread From: Christophe JAILLET @ 2023-11-05 21:27 UTC (permalink / raw) To: Kees Cook, Julia Lawall Cc: Kees Cook, Gustavo A. R. Silva, linux-kernel, kernel-janitors, linux-hardening Le 01/10/2023 à 23:22, Kees Cook a écrit : > On Sun, Oct 01, 2023 at 02:05:46PM -0700, Kees Cook wrote: >> On Sun, Oct 01, 2023 at 09:14:02PM +0200, Julia Lawall wrote: >>> Kees, >>> >>> You can try the following. >> >> Cool! Yeah, this finds the example: >> >> drivers/comedi/drivers/rti800.c:74: struct comedi_lrange: field at offset 0 is the counter for the flex array >> drivers/comedi/drivers/rti800.c:83: struct comedi_lrange: field at offset 0 is the counter for the flex array >> drivers/comedi/drivers/rti800.c:92: struct comedi_lrange: field at offset 0 is the counter for the flex array >> >> I'll run it on the whole codebase... > > It found only the struct comedi_lrange instances, but that's good to > know. :) > Hi, (removing most of people from the thread) I found another one. struct ocotp_devtype_data has a field 'num_entry' which is the number of entries in 'entry' flex array. [1]: https://elixir.bootlin.com/linux/v6.6/source/drivers/nvmem/imx-ocotp-ele.c#L28 [2]: https://elixir.bootlin.com/linux/v6.6/source/drivers/nvmem/imx-ocotp-ele.c#L143 CJ ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by 2023-09-30 9:14 [PATCH] comedi: Annotate struct comedi_lrange with __counted_by Christophe JAILLET 2023-09-30 20:57 ` Kees Cook @ 2023-10-01 6:43 ` Gustavo A. R. Silva 1 sibling, 0 replies; 14+ messages in thread From: Gustavo A. R. Silva @ 2023-10-01 6:43 UTC (permalink / raw) To: Christophe JAILLET, Ian Abbott, H Hartley Sweeten, Kees Cook, Gustavo A. R. Silva, Nathan Chancellor, Nick Desaulniers, Tom Rix Cc: linux-kernel, kernel-janitors, linux-hardening, llvm On 9/30/23 11:14, Christophe JAILLET wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> Thanks -- Gustavo > --- > This patch is part of a work done in parallel of what is currently worked > on by Kees Cook. > > My patches are only related to corner cases that do NOT match the > semantic of his Coccinelle script[1]. > > In this case, it is been spotted because of comedi_alloc_spriv(). > All other usages of struct comedi_lrange seem to be static definition of > the structure that explicitly set the .length field. > > [1] https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci > --- > include/linux/comedi/comedidev.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/comedi/comedidev.h b/include/linux/comedi/comedidev.h > index 0a1150900ef3..c08416a7364b 100644 > --- a/include/linux/comedi/comedidev.h > +++ b/include/linux/comedi/comedidev.h > @@ -633,7 +633,7 @@ extern const struct comedi_lrange range_unknown; > */ > struct comedi_lrange { > int length; > - struct comedi_krange range[]; > + struct comedi_krange range[] __counted_by(length); > }; > > /** ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-11-05 21:28 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-30 9:14 [PATCH] comedi: Annotate struct comedi_lrange with __counted_by Christophe JAILLET 2023-09-30 20:57 ` Kees Cook 2023-10-01 7:25 ` Julia Lawall 2023-10-01 7:50 ` Christophe JAILLET 2023-10-01 7:45 ` Julia Lawall 2023-10-01 15:26 ` Kees Cook 2023-10-01 19:14 ` Julia Lawall 2023-10-01 21:05 ` Kees Cook 2023-10-01 21:22 ` Kees Cook 2023-10-01 22:21 ` Kees Cook 2023-10-02 5:38 ` Julia Lawall 2023-10-02 7:36 ` Kees Cook 2023-11-05 21:27 ` Christophe JAILLET 2023-10-01 6:43 ` Gustavo A. R. Silva
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox