[PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Young Xiao]

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(+)

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;
 
-- 
1.9.1

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6913 bytes --]

Hi Young,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.1-rc4 next-20190410]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Young-Xiao/USB-s2255-stkwebcam-fix-oops-with-malicious-USB-descriptors/20190411-213648
base:   git://linuxtv.org/media_tree.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers/media/usb/s2255/s2255drv.c: In function 's2255_probe':
>> drivers/media/usb/s2255/s2255drv.c:2270:3: error: label 'error' used but not defined
      goto error;
      ^~~~
--
   drivers/media/usb/stkwebcam/stk-webcam.c: In function 'stk_camera_probe':
>> drivers/media/usb/stkwebcam/stk-webcam.c:1355:3: error: 'retval' undeclared (first use in this function); did you mean 'regval'?
      retval = -EINVAL;
      ^~~~~~
      regval
   drivers/media/usb/stkwebcam/stk-webcam.c:1355:3: note: each undeclared identifier is reported only once for each function it appears in

vim +/error +2270 drivers/media/usb/s2255/s2255drv.c

  2219	
  2220	/* standard usb probe function */
  2221	static int s2255_probe(struct usb_interface *interface,
  2222			       const struct usb_device_id *id)
  2223	{
  2224		struct s2255_dev *dev = NULL;
  2225		struct usb_host_interface *iface_desc;
  2226		struct usb_endpoint_descriptor *endpoint;
  2227		int i;
  2228		int retval = -ENOMEM;
  2229		__le32 *pdata;
  2230		int fw_size;
  2231	
  2232		/* allocate memory for our device state and initialize it to zero */
  2233		dev = kzalloc(sizeof(struct s2255_dev), GFP_KERNEL);
  2234		if (dev == NULL) {
  2235			s2255_dev_err(&interface->dev, "out of memory\n");
  2236			return -ENOMEM;
  2237		}
  2238	
  2239		dev->cmdbuf = kzalloc(S2255_CMDBUF_SIZE, GFP_KERNEL);
  2240		if (dev->cmdbuf == NULL) {
  2241			s2255_dev_err(&interface->dev, "out of memory\n");
  2242			goto errorFWDATA1;
  2243		}
  2244	
  2245		atomic_set(&dev->num_channels, 0);
  2246		dev->pid = id->idProduct;
  2247		dev->fw_data = kzalloc(sizeof(struct s2255_fw), GFP_KERNEL);
  2248		if (!dev->fw_data)
  2249			goto errorFWDATA1;
  2250		mutex_init(&dev->lock);
  2251		mutex_init(&dev->cmdlock);
  2252		/* grab usb_device and save it */
  2253		dev->udev = usb_get_dev(interface_to_usbdev(interface));
  2254		if (dev->udev == NULL) {
  2255			dev_err(&interface->dev, "null usb device\n");
  2256			retval = -ENODEV;
  2257			goto errorUDEV;
  2258		}
  2259		dev_dbg(&interface->dev, "dev: %p, udev %p interface %p\n",
  2260			dev, dev->udev, interface);
  2261		dev->interface = interface;
  2262		/* set up the endpoint information  */
  2263		iface_desc = interface->cur_altsetting;
  2264		dev_dbg(&interface->dev, "num EP: %d\n",
  2265			iface_desc->desc.bNumEndpoints);
  2266	
  2267		if (iface_desc->desc.bNumEndpoints < 1) {
  2268			dev_err(&interface->dev, "Invalid number of endpoints\n");
  2269			retval = -EINVAL;
> 2270			goto error;
  2271		}
  2272	
  2273		for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
  2274			endpoint = &iface_desc->endpoint[i].desc;
  2275			if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoint)) {
  2276				/* we found the bulk in endpoint */
  2277				dev->read_endpoint = endpoint->bEndpointAddress;
  2278			}
  2279		}
  2280	
  2281		if (!dev->read_endpoint) {
  2282			dev_err(&interface->dev, "Could not find bulk-in endpoint\n");
  2283			goto errorEP;
  2284		}
  2285		timer_setup(&dev->timer, s2255_timer, 0);
  2286		init_waitqueue_head(&dev->fw_data->wait_fw);
  2287		for (i = 0; i < MAX_CHANNELS; i++) {
  2288			struct s2255_vc *vc = &dev->vc[i];
  2289			vc->idx = i;
  2290			vc->dev = dev;
  2291			init_waitqueue_head(&vc->wait_setmode);
  2292			init_waitqueue_head(&vc->wait_vidstatus);
  2293			spin_lock_init(&vc->qlock);
  2294			mutex_init(&vc->vb_lock);
  2295		}
  2296	
  2297		dev->fw_data->fw_urb = usb_alloc_urb(0, GFP_KERNEL);
  2298		if (!dev->fw_data->fw_urb)
  2299			goto errorFWURB;
  2300	
  2301		dev->fw_data->pfw_data = kzalloc(CHUNK_SIZE, GFP_KERNEL);
  2302		if (!dev->fw_data->pfw_data) {
  2303			dev_err(&interface->dev, "out of memory!\n");
  2304			goto errorFWDATA2;
  2305		}
  2306		/* load the first chunk */
  2307		if (request_firmware(&dev->fw_data->fw,
  2308				     FIRMWARE_FILE_NAME, &dev->udev->dev)) {
  2309			dev_err(&interface->dev, "sensoray 2255 failed to get firmware\n");
  2310			goto errorREQFW;
  2311		}
  2312		/* check the firmware is valid */
  2313		fw_size = dev->fw_data->fw->size;
  2314		pdata = (__le32 *) &dev->fw_data->fw->data[fw_size - 8];
  2315	
  2316		if (*pdata != S2255_FW_MARKER) {
  2317			dev_err(&interface->dev, "Firmware invalid.\n");
  2318			retval = -ENODEV;
  2319			goto errorFWMARKER;
  2320		} else {
  2321			/* make sure firmware is the latest */
  2322			__le32 *pRel;
  2323			pRel = (__le32 *) &dev->fw_data->fw->data[fw_size - 4];
  2324			pr_info("s2255 dsp fw version %x\n", le32_to_cpu(*pRel));
  2325			dev->dsp_fw_ver = le32_to_cpu(*pRel);
  2326			if (dev->dsp_fw_ver < S2255_CUR_DSP_FWVER)
  2327				pr_info("s2255: f2255usb.bin out of date.\n");
  2328			if (dev->pid == 0x2257 &&
  2329					dev->dsp_fw_ver < S2255_MIN_DSP_COLORFILTER)
  2330				pr_warn("2257 needs firmware %d or above.\n",
  2331					S2255_MIN_DSP_COLORFILTER);
  2332		}
  2333		usb_reset_device(dev->udev);
  2334		/* load 2255 board specific */
  2335		retval = s2255_board_init(dev);
  2336		if (retval)
  2337			goto errorBOARDINIT;
  2338		s2255_fwload_start(dev);
  2339		/* loads v4l specific */
  2340		retval = s2255_probe_v4l(dev);
  2341		if (retval)
  2342			goto errorBOARDINIT;
  2343		dev_info(&interface->dev, "Sensoray 2255 detected\n");
  2344		return 0;
  2345	errorBOARDINIT:
  2346		s2255_board_shutdown(dev);
  2347	errorFWMARKER:
  2348		release_firmware(dev->fw_data->fw);
  2349	errorREQFW:
  2350		kfree(dev->fw_data->pfw_data);
  2351	errorFWDATA2:
  2352		usb_free_urb(dev->fw_data->fw_urb);
  2353	errorFWURB:
  2354		del_timer_sync(&dev->timer);
  2355	errorEP:
  2356		usb_put_dev(dev->udev);
  2357	errorUDEV:
  2358		kfree(dev->fw_data);
  2359		mutex_destroy(&dev->lock);
  2360	errorFWDATA1:
  2361		kfree(dev->cmdbuf);
  2362		kfree(dev);
  2363		pr_warn("Sensoray 2255 driver load failed: 0x%x\n", retval);
  2364		return retval;
  2365	}
  2366	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56101 bytes --]

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Greg KH]

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

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Johan Hovold]

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

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Johan Hovold]

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] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Young Xiao]

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(+)

diff --git a/drivers/media/usb/s2255/s2255drv.c b/drivers/media/usb/s2255/s2255drv.c
index 5b3e54b..82dd661 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 errorEP;
+	}
+
 	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..e427c3d 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");
+		err = -EINVAL;
+		goto error;
+	}
+
 	for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
 		endpoint = &iface_desc->endpoint[i].desc;
 
-- 
1.9.1

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Bjørn Mork]

Please mark updated patches with a version number or some other
indication that it replaces a previous patch.  Including a summary of
changes is also normal.

And speaking of normal:  We do build test our patches, don't we?


Young Xiao <92siuyang@gmail.com> writes:

> 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.

No, it won't.  Did you test this?  Can you provide the oops?

This is perfectly fine as it is:

	dev = kzalloc(sizeof(struct s2255_dev), GFP_KERNEL);
..
	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)) {
			/* we found the bulk in endpoint */
			dev->read_endpoint = endpoint->bEndpointAddress;
		}
	}

	if (!dev->read_endpoint) {
		dev_err(&interface->dev, "Could not find bulk-in endpoint\n");
		goto errorEP;
	}

>  drivers/media/usb/stkwebcam/stk-webcam.c | 6 ++++++

I didn't bother looking at this driver to see if your patch there makes
more sense.  That is your home work now.  Please explain why you believe
it is.  An actual oops would be good.

<rant>
Yes, and I do have some objections to this whole "protect against
malicious devices".  How do you intend to protect against a USB device
disguising itself as a keyboard or ethernet adapater or whatever?
Allowing potentionally malicious devices is crazy enough for USB, and it
gets completely wacko when people start talking about it in the context
of firewire or PCIe

Fixing bugs in drivers is fine. But it won't make any difference wrt
security if you connect malicious devices to your system.  Don't do that
if you want a secure system.

Allocating CVE numbers to arbitrary driver bugs is just adding
noise. This noise makes it harder for sysadmins and others to to notice
the really important problems.  No one will care anymore if every kernel
release fixes thousands of CVEs.  Which is pretty close to the truth if
you start allocating CVE numbers to any bug with a security impact.
</rant>




Bjørn

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Yang Xiao]

Hello,

Thanks  for your response, firstly.

The affected version ranges from v3.7 to v5.1.

-------------------------------------------------------------
Below is the analysis of the vulnerability:

As said in the comment, the driver expects at least one valid endpoint.
If given malicious descritors that spcify 0 for the number of
endpoints, then there is a null pointer deference when calling
function usb_endpoint_is_bulk_in.

       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)) {
                        /* we found the bulk in endpoint */
                        dev->read_endpoint = endpoint->bEndpointAddress;
                }
        }

static inline int usb_endpoint_is_bulk_in(
const struct usb_endpoint_descriptor *epd)
{
return usb_endpoint_xfer_bulk(epd) && usb_endpoint_dir_in(epd);
}

static inline int usb_endpoint_xfer_bulk(
const struct usb_endpoint_descriptor *epd)
{
return ((epd->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) ==
USB_ENDPOINT_XFER_BULK);
}

There is a call to function usb_endpoint_is_bulk_in after assignment
to endpoint.
And the field bmAttributes is accessed in function
usb_endpoint_xfer_bulk (usb_endpoint_is_bulk_in ->
usb_endpoint_xfer_bulk).
Since the number of descriptors is 0, endpoint is assignment to NULL.
Then NULL pointer deference in function usb_endpoint_xfer_bulk (oops).

If you insist on a PoC, sorry for that. I found the vulnerability by
analyzing the code staticlly.

Below is the reply of your rant:
Everyone wants to build a secury Linux kernel with different ways.
Fuzzing, static analysis are all good ways.
And there are many missing fixes when a vulnerability is found indeed,
since there are much code clone in codebase.

I agree with you on your explain about alllocating CVE numbers to
arbitrary driver bugs.
Complaint is useless. As main developer of kernel, I think you can
disscuss the problem with other main developers.
There should be a baseline. Which vulnerabilities should be assigned
with a CVE number, and which should not.

However, if there are real bugs or vulnerabilities, we still need to
fix them, don't we?

Besides, I am sorry for not explaining the patch clearly when I
submitted the patch. I will try to analyse the possible vulnerability
when submitting patches next time.

Young


On Fri, Apr 12, 2019 at 4:04 PM Bjørn Mork <bjorn@mork.no> wrote:
>
> Please mark updated patches with a version number or some other
> indication that it replaces a previous patch.  Including a summary of
> changes is also normal.
>
> And speaking of normal:  We do build test our patches, don't we?
>
>
> Young Xiao <92siuyang@gmail.com> writes:
>
> > 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.
>
> No, it won't.  Did you test this?  Can you provide the oops?
>
> This is perfectly fine as it is:
>
>         dev = kzalloc(sizeof(struct s2255_dev), GFP_KERNEL);
> ..
>         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)) {
>                         /* we found the bulk in endpoint */
>                         dev->read_endpoint = endpoint->bEndpointAddress;
>                 }
>         }
>
>         if (!dev->read_endpoint) {
>                 dev_err(&interface->dev, "Could not find bulk-in endpoint\n");
>                 goto errorEP;
>         }
>
> >  drivers/media/usb/stkwebcam/stk-webcam.c | 6 ++++++
>
> I didn't bother looking at this driver to see if your patch there makes
> more sense.  That is your home work now.  Please explain why you believe
> it is.  An actual oops would be good.
>
> <rant>
> Yes, and I do have some objections to this whole "protect against
> malicious devices".  How do you intend to protect against a USB device
> disguising itself as a keyboard or ethernet adapater or whatever?
> Allowing potentionally malicious devices is crazy enough for USB, and it
> gets completely wacko when people start talking about it in the context
> of firewire or PCIe
>
> Fixing bugs in drivers is fine. But it won't make any difference wrt
> security if you connect malicious devices to your system.  Don't do that
> if you want a secure system.
>
> Allocating CVE numbers to arbitrary driver bugs is just adding
> noise. This noise makes it harder for sysadmins and others to to notice
> the really important problems.  No one will care anymore if every kernel
> release fixes thousands of CVEs.  Which is pretty close to the truth if
> you start allocating CVE numbers to any bug with a security impact.
> </rant>
>
>
>
>
> Bjørn



-- 
Best regards!

Young
-----------------------------------------------------------

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Bjørn Mork]

Yang Xiao <92siuyang@gmail.com> writes:

> If given malicious descritors that spcify 0 for the number of endpoints,
> then there is a null pointer deference when calling function
> usb_endpoint_is_bulk_in.
>
>        for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {

Try this:

#include <stdio.h>
int main()
{
        int i;
        for (i=0; i<0; ++i)
                printf("%d\n");
        return 0;
}

How many lines did it print?


Bjørn

Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

[Yang Xiao]

I am so sorry. I misunderstood the reason of  CVE-2016-2188.

Sorry again!!!

On Fri, Apr 12, 2019 at 5:07 PM Bjørn Mork <bjorn@mork.no> wrote:
>
> Yang Xiao <92siuyang@gmail.com> writes:
>
> > If given malicious descritors that spcify 0 for the number of endpoints,
> > then there is a null pointer deference when calling function
> > usb_endpoint_is_bulk_in.
> >
> >        for (i = 0; i < iface_desc->desc.bNumEndpoints; ++i) {
>
> Try this:
>
> #include <stdio.h>
> int main()
> {
>         int i;
>         for (i=0; i<0; ++i)
>                 printf("%d\n");
>         return 0;
> }
>
> How many lines did it print?
>
>
> Bjørn