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@xxxxxxx> 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@xxxxxxxxx> writes:
>
> > From: Young Xiao <YangX92@xxxxxxxxxxx>
> >
> > 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
-----------------------------------------------------------
