Message ID | ZRbwU8Qnx28gpbuO@work (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1qmFdF-000t3y-KK; Fri, 29 Sep 2023 15:42:21 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233517AbjI2PmU (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 29 Sep 2023 11:42:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58362 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231429AbjI2PmT (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 29 Sep 2023 11:42:19 -0400 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1D3A8B4; Fri, 29 Sep 2023 08:42:17 -0700 (PDT) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 69593C433C7; Fri, 29 Sep 2023 15:42:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1696002136; bh=Zl9IzDfJ74tSHG7Bo3AkrSgoNzTjNILJUvTaD+C2hpM=; h=Date:From:To:Cc:Subject:From; b=qyDMdW5b+k9YsTcDHVa6jtbOihi/nlM/xncg5bZPJCRQEuUEtK6jXE+OoW58OSQKD ZgwbxqXxdZAEhgaBpWo1c6XxZXkhcZxQDzvkVFpR2DU9DUyFLlUaKG82/Y8YTNd8xi O0d0hVUy/uqd4PbDUTXeIbpEcm8etsmdMnXgFmi3YG/Rq2A8LhFPDcVj5uCKaLcWOc ohsuJ5/hPAQT39aQW/NCjmO153g/RPL8acTQLExpvutAuJQEAvtDUH8TEmcY4LJcTv rMg8SZZn5zoIF+WcA/LUJT8UlLl173AXx+XZ1ysj4XwrpKtIvsUSoTcTAcC9B13QIb /KAO3+pqJztOQ== Date: Fri, 29 Sep 2023 17:42:11 +0200 From: "Gustavo A. R. Silva" <gustavoars@kernel.org> To: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" <gustavoars@kernel.org>, linux-hardening@vger.kernel.org Subject: [PATCH][next] media: usb: siano: Fix undefined behavior bug in struct smsusb_urb_t Message-ID: <ZRbwU8Qnx28gpbuO@work> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -3.1 (---) X-LSpam-Report: No, score=-3.1 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,DKIM_VALID_EF=-0.1,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
[next] media: usb: siano: Fix undefined behavior bug in struct smsusb_urb_t
|
|
Commit Message
Gustavo A. R. Silva
Sept. 29, 2023, 3:42 p.m. UTC
`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@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(-)
Comments
On Fri, Sep 29, 2023 at 5:42 PM 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 structures * 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/smsusb.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 > >
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@vger.kernel.org > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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. Reviewed-by: Kees Cook <keescook@chromium.org>
On Fri, Sep 29, 2023 at 7:29 PM Kees Cook <keescook@chromium.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@vger.kernel.org > > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.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.
On Fri, Sep 29, 2023 at 06:20:10PM +0200, Jann Horn wrote: > On Fri, Sep 29, 2023 at 5:42 PM 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 structures > * 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/smsusb.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; > > }; Yeah, this is going to get messy. Ideally, just dynamically create the urb and change this to a "struct urb *urb;" instead. thanks, greg k-h
On 9/30/23 09:01, Greg Kroah-Hartman wrote: > On Fri, Sep 29, 2023 at 06:20:10PM +0200, Jann Horn wrote: >> On Fri, Sep 29, 2023 at 5:42 PM 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 Yeah. I noticed that too. Probably what Greg suggests (dynamically create the urb) can fix this, too. I haven't taken a deep dive into this particular case. So, let me go and figure something out. >> 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 structures >> * 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/smsusb.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; >>> }; > > Yeah, this is going to get messy. Ideally, just dynamically create the > urb and change this to a "struct urb *urb;" instead. Probably, yes. Thanks -- Gustavo
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.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 {