Patchwork USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

login
register
mail settings
Submitter Young Xiao
Date April 11, 2019, 4:54 a.m.
Message ID <1554958452-29794-1-git-send-email-92siuyang@gmail.com>
Download mbox | patch
Permalink /patch/770261/
State New
Headers show

Comments

Young Xiao - April 11, 2019, 4:54 a.m.
From: Young Xiao <YangX92@hotmail.com>

The driver expects at least one valid endpoint. If given
malicious descriptors that specify 0 for the number of endpoints,
it will crash in the probe function.  Ensure there is at least
one endpoint on the interface before using it.

This vulnerability is same as CVE-2016-2188.

Signed-off-by: Young Xiao <YangX92@hotmail.com>
---
 drivers/media/usb/s2255/s2255drv.c       | 7 +++++++
 drivers/media/usb/stkwebcam/stk-webcam.c | 6 ++++++
 2 files changed, 13 insertions(+)
Greg KH - April 16, 2019, 10:06 a.m.
On Fri, Apr 12, 2019 at 10:14:50AM +0800, Yang Xiao wrote:
> Sorry for my mistake.
> 
> I have fixed the mistake. And the attachment is the changed patch.

I can not apply attachments, please fix up and resend as documented.

thanks,

greg k-h
Johan Hovold - April 16, 2019, 11:26 a.m.
On Thu, Apr 11, 2019 at 12:54:12PM +0800, Young Xiao wrote:
> From: Young Xiao <YangX92@hotmail.com>
> 
> The driver expects at least one valid endpoint. If given
> malicious descriptors that specify 0 for the number of endpoints,
> it will crash in the probe function.  Ensure there is at least
> one endpoint on the interface before using it.

Why do claim it will crash?

> This vulnerability is same as CVE-2016-2188.

Note that the "fix" for this CVE that you're now copying was incomplete.
Here's the proper fix:

	b7321e81fc36 ("USB: iowarrior: fix NULL-deref at probe")

> Signed-off-by: Young Xiao <YangX92@hotmail.com>
> ---
>  drivers/media/usb/s2255/s2255drv.c       | 7 +++++++
>  drivers/media/usb/stkwebcam/stk-webcam.c | 6 ++++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
> index 5b3e54b..7fdf159 100644
> --- a/drivers/media/usb/s2255/s2255drv.c
> +++ b/drivers/media/usb/s2255/s2255drv.c
> @@ -2263,6 +2263,13 @@ static int s2255_probe(struct usb_interface *interface,
>  	iface_desc = interface->cur_altsetting;
>  	dev_dbg(&interface->dev, "num EP: %d\n",
>  		iface_desc->desc.bNumEndpoints);
> +
> +	if (iface_desc->desc.bNumEndpoints < 1) {
> +		dev_err(&interface->dev, "Invalid number of endpoints\n");
> +		retval = -EINVAL;
> +		goto error;
> +	}
> +
>  	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {

Besides that you didn't even bother compile-testing this, there is no
bug here to fix to begin with.

If bNumEndpoints is zero this loop will execute and the driver bails out
just after since dev->read_endpoint is NULL.

>  		endpoint = &iface_desc->endpoint[i].desc;
>  		if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoint)) {
> diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
> index 8f54586..d2a4785 100644
> --- a/drivers/media/usb/stkwebcam/stk-webcam.c
> +++ b/drivers/media/usb/stkwebcam/stk-webcam.c
> @@ -1350,6 +1350,12 @@ static int stk_camera_probe(struct usb_interface *interface,
>  	 * for the current alternate setting */
>  	iface_desc = interface->cur_altsetting;
>  
> +	if (iface_desc->desc.bNumEndpoints < 1) {
> +		dev_err(&interface->dev, "Invalid number of endpoints\n");
> +		retval = -EINVAL;
> +		goto error;
> +	}
> +
>  	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {

Same here.

>  		endpoint = &iface_desc->endpoint[i].desc;

Johan
Johan Hovold - April 16, 2019, 11:33 a.m.
On Tue, Apr 16, 2019 at 01:26:45PM +0200, Johan Hovold wrote:
> On Thu, Apr 11, 2019 at 12:54:12PM +0800, Young Xiao wrote:
> > From: Young Xiao <YangX92@hotmail.com>
> > 
> > The driver expects at least one valid endpoint. If given
> > malicious descriptors that specify 0 for the number of endpoints,
> > it will crash in the probe function.  Ensure there is at least
> > one endpoint on the interface before using it.
> 
> Why do claim it will crash?

Ok, I see now that Björn already pointed this out to you in your
updated version of this patch.

> > This vulnerability is same as CVE-2016-2188.
> 
> Note that the "fix" for this CVE that you're now copying was incomplete.
> Here's the proper fix:
> 
> 	b7321e81fc36 ("USB: iowarrior: fix NULL-deref at probe")
> 
> > Signed-off-by: Young Xiao <YangX92@hotmail.com>
> > ---
> >  drivers/media/usb/s2255/s2255drv.c       | 7 +++++++
> >  drivers/media/usb/stkwebcam/stk-webcam.c | 6 ++++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
> > index 5b3e54b..7fdf159 100644
> > --- a/drivers/media/usb/s2255/s2255drv.c
> > +++ b/drivers/media/usb/s2255/s2255drv.c
> > @@ -2263,6 +2263,13 @@ static int s2255_probe(struct usb_interface *interface,
> >  	iface_desc = interface->cur_altsetting;
> >  	dev_dbg(&interface->dev, "num EP: %d\n",
> >  		iface_desc->desc.bNumEndpoints);
> > +
> > +	if (iface_desc->desc.bNumEndpoints < 1) {
> > +		dev_err(&interface->dev, "Invalid number of endpoints\n");
> > +		retval = -EINVAL;
> > +		goto error;
> > +	}
> > +
> >  	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
> 
> Besides that you didn't even bother compile-testing this, there is no
> bug here to fix to begin with.
> 
> If bNumEndpoints is zero this loop will execute and the driver bails out
> just after since dev->read_endpoint is NULL.

That was meant to read "will never execute".

Johan

Patch

diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
index 5b3e54b..7fdf159 100644
--- a/drivers/media/usb/s2255/s2255drv.c
+++ b/drivers/media/usb/s2255/s2255drv.c
@@ -2263,6 +2263,13 @@  static int s2255_probe(struct usb_interface *interface,
 	iface_desc = interface->cur_altsetting;
 	dev_dbg(&interface->dev, "num EP: %d\n",
 		iface_desc->desc.bNumEndpoints);
+
+	if (iface_desc->desc.bNumEndpoints < 1) {
+		dev_err(&interface->dev, "Invalid number of endpoints\n");
+		retval = -EINVAL;
+		goto error;
+	}
+
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
 		endpoint = &iface_desc->endpoint[i].desc;
 		if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoint)) {
diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index 8f54586..d2a4785 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -1350,6 +1350,12 @@  static int stk_camera_probe(struct usb_interface *interface,
 	 * for the current alternate setting */
 	iface_desc = interface->cur_altsetting;
 
+	if (iface_desc->desc.bNumEndpoints < 1) {
+		dev_err(&interface->dev, "Invalid number of endpoints\n");
+		retval = -EINVAL;
+		goto error;
+	}
+
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
 		endpoint = &iface_desc->endpoint[i].desc;