| Message ID | 5c3b7459b820e22e2ac6ce892d4aadcc119cc919.1696065263.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 77f048bcbf07f7dc961f3b2b7815038b5405ec60 |
| Headers | show |
| Series | comedi: Annotate struct comedi_lrange with __counted_by | expand |
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 >
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
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 >
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 >
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 > >
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,
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 *)
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...
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 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
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
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. :)
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
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); }; /**
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(-)