| 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: <20190524103540.250a69e7@coco.lan>
Date: Fri, 24 May 2019 10:35:40 -0300
From: Mauro Carvalho Chehab <mchehab@...nel.org>
To: Alan Stern <stern@...land.harvard.edu>
Cc: andreyknvl@...gle.com,
Kernel development list <linux-kernel@...r.kernel.org>,
<linux-media@...r.kernel.org>,
USB list <linux-usb@...r.kernel.org>,
<syzkaller-bugs@...glegroups.com>, <wen.yang99@....com.cn>
Subject: Re: [PATCH] media: usb: siano: Fix general protection fault in
smsusb
Em Tue, 7 May 2019 12:39:47 -0400 (EDT)
Alan Stern <stern@...land.harvard.edu> escreveu:
> The syzkaller USB fuzzer found a general-protection-fault bug in the
> smsusb part of the Siano DVB driver. The fault occurs during probe
> because the driver assumes without checking that the device has both
> IN and OUT endpoints and the IN endpoint is ep1.
>
> By slightly rearranging the driver's initialization code, we can make
> the appropriate checks early on and thus avoid the problem. If the
> expected endpoints aren't present, the new code safely returns -ENODEV
> from the probe routine.
>
> Signed-off-by: Alan Stern <stern@...land.harvard.edu>
> Reported-and-tested-by: syzbot+53f029db71c19a47325a@...kaller.appspotmail.com
> CC: <stable@...r.kernel.org>
>
> ---
>
>
> [as1897]
>
>
> drivers/media/usb/siano/smsusb.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
>
> Index: usb-devel/drivers/media/usb/siano/smsusb.c
> ===================================================================
> --- usb-devel.orig/drivers/media/usb/siano/smsusb.c
> +++ usb-devel/drivers/media/usb/siano/smsusb.c
> @@ -400,6 +400,7 @@ static int smsusb_init_device(struct usb
> struct smsusb_device_t *dev;
> void *mdev;
> int i, rc;
> + int in_maxp;
>
> /* create device object */
> dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
> @@ -411,6 +412,24 @@ static int smsusb_init_device(struct usb
> dev->udev = interface_to_usbdev(intf);
> dev->state = SMSUSB_DISCONNECTED;
>
> + for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> + struct usb_endpoint_descriptor *desc =
> + &intf->cur_altsetting->endpoint[i].desc;
> +
> + if (desc->bEndpointAddress & USB_DIR_IN) {
> + dev->in_ep = desc->bEndpointAddress;
> + in_maxp = usb_endpoint_maxp(desc);
> + } else {
> + dev->out_ep = desc->bEndpointAddress;
> + }
> + }
> +
> + pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep);
> + if (!dev->in_ep || !dev->out_ep) { /* Missing endpoints? */
> + smsusb_term_device(intf);
> + return -ENODEV;
> + }
> +
> params.device_type = sms_get_board(board_id)->type;
>
> switch (params.device_type) {
> @@ -425,24 +444,12 @@ static int smsusb_init_device(struct usb
> /* fall-thru */
> default:
> dev->buffer_size = USB2_BUFFER_SIZE;
> - dev->response_alignment =
> - le16_to_cpu(dev->udev->ep_in[1]->desc.wMaxPacketSize) -
> - sizeof(struct sms_msg_hdr);
> + dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
>
> params.flags |= SMS_DEVICE_FAMILY2;
> break;
> }
>
> - for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> - if (intf->cur_altsetting->endpoint[i].desc. bEndpointAddress & USB_DIR_IN)
> - dev->in_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
> - else
> - dev->out_ep = intf->cur_altsetting->endpoint[i].desc.bEndpointAddress;
> - }
> -
> - pr_debug("in_ep = %02x, out_ep = %02x\n",
> - dev->in_ep, dev->out_ep);
> -
> params.device = &dev->udev->dev;
> params.usb_device = dev->udev;
> params.buffer_size = dev->buffer_size;
>
Patch looks correct, and I'm applying it. It exposes another potential
problem though: what happens if sizeof(desc.wMaxPacketSize) < sizeof(struct sms_msg_hdr)?
I'm enclosing a followup patch that should solve this situation
(and clean up a sparse warning).
Thanks,
Mauro
[PATCH] media: smsusb: better handle optional alignment
Most Siano devices require an alignment for the response.
Changeset f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb")
changed the logic with gets such aligment, but it now produces a
sparce warning:
drivers/media/usb/siano/smsusb.c: In function 'smsusb_init_device':
drivers/media/usb/siano/smsusb.c:447:37: warning: 'in_maxp' may be used uninitialized in this function [-Wmaybe-uninitialized]
447 | dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
| ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
The sparse message itself is bogus, but a broken (or fake) USB
eeprom could produce a negative value for response_alignment.
So, change the code in order to check if the result is not
negative.
Fixes: f3be52b0056a ("media: usb: siano: Fix general protection fault in smsusb")
CC: <stable@...r.kernel.org>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@...nel.org>
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
index 27ad14a3f831..e39f3f40dfdd 100644
--- a/drivers/media/usb/siano/smsusb.c
+++ b/drivers/media/usb/siano/smsusb.c
@@ -400,7 +400,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
struct smsusb_device_t *dev;
void *mdev;
int i, rc;
- int in_maxp;
+ int align = 0;
/* create device object */
dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL);
@@ -418,14 +418,14 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
if (desc->bEndpointAddress & USB_DIR_IN) {
dev->in_ep = desc->bEndpointAddress;
- in_maxp = usb_endpoint_maxp(desc);
+ align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr);
} else {
dev->out_ep = desc->bEndpointAddress;
}
}
pr_debug("in_ep = %02x, out_ep = %02x\n", dev->in_ep, dev->out_ep);
- if (!dev->in_ep || !dev->out_ep) { /* Missing endpoints? */
+ if (!dev->in_ep || !dev->out_ep || align < 0) { /* Missing endpoints? */
smsusb_term_device(intf);
return -ENODEV;
}
@@ -444,7 +444,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
/* fall-thru */
default:
dev->buffer_size = USB2_BUFFER_SIZE;
- dev->response_alignment = in_maxp - sizeof(struct sms_msg_hdr);
+ dev->response_alignment = align;
params.flags |= SMS_DEVICE_FAMILY2;
break;
Powered by blists - more mailing lists