Re: [PATCH] comedi: Annotate struct comedi_lrange with __counted_by

[Julia Lawall]

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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx/
> But that above for stack varaibles rather than globals. But I'm way off
> topic now. ;)
>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> >
> > [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
>