linux-hardening - Re: [PATCH][next] media: usb: siano: Fix undefined behavior bug in struct smsusb_urb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez1w_gBUF6qRmfO_wb+eYLTfrLs5csjEaPhE9iQxdqPj+Q@mail.gmail.com>
Date: Fri, 29 Sep 2023 19:40:07 +0200
From: Jann Horn <jannh@...gle.com>
To: Kees Cook <keescook@...omium.org>
Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] media: usb: siano: Fix undefined behavior bug in
 struct smsusb_urb_t

On Fri, Sep 29, 2023 at 7:29 PM Kees Cook <keescook@...omium.org> 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@...r.kernel.org
> > Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
>
> 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.