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
| ||
|
Message-ID: <d81a2187-a2ff-c741-8f35-a1af54652ebd@embeddedor.com> Date: Sun, 1 Oct 2023 08:58:00 +0200 From: "Gustavo A. R. Silva" <gustavo@...eddedor.com> To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jann Horn <jannh@...gle.com> Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>, USB list <linux-usb@...r.kernel.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 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@...nel.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@...r.kernel.org >>> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.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
Powered by blists - more mailing lists