* [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
@ 2019-04-12 2:39 Young Xiao
2019-04-12 8:04 ` Bjørn Mork
0 siblings, 1 reply; 10+ messages in thread
From: Young Xiao @ 2019-04-12 2:39 UTC (permalink / raw)
To: kbuild-all, linux-usb, linux-media, linux-kernel, greg, mchehab
Cc: keescook, hans.verkuil, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-12 2:39 [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors Young Xiao
@ 2019-04-12 8:04 ` Bjørn Mork
2019-04-12 8:58 ` Yang Xiao
[not found] ` <CAKgHYH05R2CQ1XmS-KCTtL0J49D2kpnkBgyYxdPc47SNpaf8vA@mail.gmail.com>
0 siblings, 2 replies; 10+ messages in thread
From: Bjørn Mork @ 2019-04-12 8:04 UTC (permalink / raw)
To: Young Xiao
Cc: kbuild-all, linux-usb, linux-media, linux-kernel, greg, mchehab,
keescook, hans.verkuil, Young Xiao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-12 8:04 ` Bjørn Mork
@ 2019-04-12 8:58 ` Yang Xiao
[not found] ` <CAKgHYH05R2CQ1XmS-KCTtL0J49D2kpnkBgyYxdPc47SNpaf8vA@mail.gmail.com>
1 sibling, 0 replies; 10+ messages in thread
From: Yang Xiao @ 2019-04-12 8:58 UTC (permalink / raw)
To: linux-usb; +Cc: linux-media, linux-kernel
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
-----------------------------------------------------------
^ permalink raw reply [flat|nested] 10+ messages in thread
[parent not found: <CAKgHYH05R2CQ1XmS-KCTtL0J49D2kpnkBgyYxdPc47SNpaf8vA@mail.gmail.com>]
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
[not found] ` <CAKgHYH05R2CQ1XmS-KCTtL0J49D2kpnkBgyYxdPc47SNpaf8vA@mail.gmail.com>
@ 2019-04-12 9:07 ` Bjørn Mork
2019-04-12 9:36 ` Yang Xiao
0 siblings, 1 reply; 10+ messages in thread
From: Bjørn Mork @ 2019-04-12 9:07 UTC (permalink / raw)
To: Yang Xiao
Cc: kbuild-all, linux-usb, linux-media, linux-kernel, greg, mchehab,
Kees Cook, hans.verkuil, Young Xiao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-12 9:07 ` Bjørn Mork
@ 2019-04-12 9:36 ` Yang Xiao
0 siblings, 0 replies; 10+ messages in thread
From: Yang Xiao @ 2019-04-12 9:36 UTC (permalink / raw)
To: Bjørn Mork
Cc: kbuild-all, linux-usb, linux-media, linux-kernel, greg, mchehab,
Kees Cook, hans.verkuil, Young 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
@ 2019-04-11 4:54 Young Xiao
2019-04-11 14:36 ` kbuild test robot
2019-04-16 11:26 ` Johan Hovold
0 siblings, 2 replies; 10+ messages in thread
From: Young Xiao @ 2019-04-11 4:54 UTC (permalink / raw)
To: linux-usb, linux-media, linux-kernel, greg
Cc: mchehab, keescook, hans.verkuil, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-11 4:54 Young Xiao
@ 2019-04-11 14:36 ` kbuild test robot
[not found] ` <CAKgHYH27TtpbUKJin+WyTRUvqgpTSBRWBTp6YhsUCpVTnKfNPA@mail.gmail.com>
2019-04-16 11:26 ` Johan Hovold
1 sibling, 1 reply; 10+ messages in thread
From: kbuild test robot @ 2019-04-11 14:36 UTC (permalink / raw)
To: Young Xiao
Cc: kbuild-all, linux-usb, linux-media, linux-kernel, greg, mchehab,
keescook, hans.verkuil, Young Xiao
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-11 4:54 Young Xiao
2019-04-11 14:36 ` kbuild test robot
@ 2019-04-16 11:26 ` Johan Hovold
2019-04-16 11:33 ` Johan Hovold
1 sibling, 1 reply; 10+ messages in thread
From: Johan Hovold @ 2019-04-16 11:26 UTC (permalink / raw)
To: Young Xiao
Cc: linux-usb, linux-media, linux-kernel, greg, mchehab, keescook,
hans.verkuil, Young Xiao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
2019-04-16 11:26 ` Johan Hovold
@ 2019-04-16 11:33 ` Johan Hovold
0 siblings, 0 replies; 10+ messages in thread
From: Johan Hovold @ 2019-04-16 11:33 UTC (permalink / raw)
To: Young Xiao
Cc: linux-usb, linux-media, linux-kernel, greg, mchehab, keescook,
hans.verkuil, Young Xiao
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-16 11:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-12 2:39 [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors Young Xiao
2019-04-12 8:04 ` Bjørn Mork
2019-04-12 8:58 ` Yang Xiao
[not found] ` <CAKgHYH05R2CQ1XmS-KCTtL0J49D2kpnkBgyYxdPc47SNpaf8vA@mail.gmail.com>
2019-04-12 9:07 ` Bjørn Mork
2019-04-12 9:36 ` Yang Xiao
-- strict thread matches above, loose matches on Subject: below --
2019-04-11 4:54 Young Xiao
2019-04-11 14:36 ` kbuild test robot
[not found] ` <CAKgHYH27TtpbUKJin+WyTRUvqgpTSBRWBTp6YhsUCpVTnKfNPA@mail.gmail.com>
2019-04-16 10:06 ` Greg KH
2019-04-16 11:26 ` Johan Hovold
2019-04-16 11:33 ` Johan Hovold
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).