From: Young Xiao <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]> writes:
> From: Young Xiao <[email protected]>
>
> 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
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 <[email protected]> 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 <[email protected]> writes:
>
> > From: Young Xiao <[email protected]>
> >
> > 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
-----------------------------------------------------------
Yang Xiao <[email protected]> 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
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 <[email protected]> wrote:
>
> Yang Xiao <[email protected]> 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
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