Re: [PATCH][next] media: usb: siano: Fix undefined behavior bug in struct smsusb_urb_t

[Jann Horn]

On Fri, Sep 29, 2023 at 7:29 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Fri, Sep 29, 2023 at 05:42:11PM +0200, Gustavo A. R. Silva wrote:
> > `struct urb` is a flexible structure, which means that it contains a
> > flexible-array member at the bottom. This could potentially lead to an
> > overwrite of the object `wq` at run-time with the contents of `urb`.
> >
> > Fix this by placing object `urb` at the end of `struct smsusb_urb_t`.
> >
> > Fixes: dd47fbd40e6e ("[media] smsusb: don't sleep while atomic")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
>
> As Jann pointed out, it's unlikely there is a function bug here, but I
> still think it's right to make sure this is robust and clears the way
> for -Wflex-array-member-not-at-end.

But if this change makes the warning go away, that just means the
warning is implemented badly, right?

Like, before we had:

struct urb {
  ...
  struct usb_iso_packet_descriptor iso_frame_desc[];
};
struct smsusb_urb_t {
  ...
  struct urb urb; // not last element
  ...
};

whereas afterwards we have:

struct urb {
  ...
  struct usb_iso_packet_descriptor iso_frame_desc[];
};
struct smsusb_urb_t {
  ...
  struct urb urb;
};
struct smsusb_device_t {
  ...
  struct smsusb_urb_t surbs[MAX_URBS]; // array, and not last element
  ...
};

That's basically the same pattern! Except that the new version has one
more layer of indirection and there's an array involved.

And you can't address that by moving struct members around because of
the involvement of the array.