Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
MIME-Version: 1.0
References: <ZRbwU8Qnx28gpbuO@work>
In-Reply-To: <ZRbwU8Qnx28gpbuO@work>
From:   Jann Horn <jannh@google.com>
Date:   Fri, 29 Sep 2023 18:20:10 +0200
Message-ID: <CAG48ez2SJMJSYrJQ9RVC44hbj3uNYBZeN0yfxWa7pqX9Fp2L7g@mail.gmail.com>
Subject: Re: [PATCH][next] media: usb: siano: Fix undefined behavior bug in
 struct smsusb_urb_t
To:     "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        USB list <linux-usb@vger.kernel.org>
Cc:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hardening@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Sep 29, 2023 at 5:42=E2=80=AFPM Gustavo A. R. Silva
<gustavoars@kernel.org> 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`.

Does this really change the situation? "struct smsusb_device_t"
contains an array of "struct smsusb_urb_t", so it seems to be like
you're just shifting the "VLA inside a non-final member of a struct"
thing around so that there is one more layer of abstraction in
between.

Comments on "struct urb" say:

 * Isochronous URBs have a different data transfer model, in part because
 * the quality of service is only "best effort".  Callers provide specially
 * allocated URBs, with number_of_packets worth of iso_frame_desc structure=
s
 * at the end.

and:

/* (in) ISO ONLY */

And it looks like smsusb only uses that URB as a bulk URB, so the flex
array is unused and we can't have an overflow here?

If this is intended to make it possible to enable some kinda compiler
warning, it might be worth talking to the USB folks to figure out the
right approach here.

> Fixes: dd47fbd40e6e ("[media] smsusb: don't sleep while atomic")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> ---
>  drivers/media/usb/siano/smsusb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/s=
msusb.c
> index 9d9e14c858e6..2c048f8e8371 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
> @@ -40,10 +40,10 @@ struct smsusb_urb_t {
>         struct smscore_buffer_t *cb;
>         struct smsusb_device_t *dev;
>
> -       struct urb urb;
> -
>         /* For the bottom half */
>         struct work_struct wq;
> +
> +       struct urb urb;
>  };
>
>  struct smsusb_device_t {
> --
> 2.34.1
>
>