| Message ID | 1555036767-31170-1-git-send-email-92siuyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | USB: s2255 & stkwebcam: fix oops with malicious USB descriptors | expand |
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
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
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
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
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;