Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1555036767-31170-1-git-send-email-92siuyang@gmail.com> <878swf645i.fsf@miraculix.mork.no>
In-Reply-To: <878swf645i.fsf@miraculix.mork.no>
From:   Yang Xiao <92siuyang@gmail.com>
Date:   Fri, 12 Apr 2019 16:58:43 +0800
Message-ID: <CAKgHYH2vwWg=3KauUNZ98QHyPNOYqbbjc5-B58mo=SyoUoyouA@mail.gmail.com>
Subject: Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors
To:     linux-usb@vger.kernel.org
Cc:     linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 =3D 0; i < iface_desc->desc.bNumEndpoints; ++i) {
                endpoint =3D &iface_desc->endpoint[i].desc;
                if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoint=
)) {
                        /* we found the bulk in endpoint */
                        dev->read_endpoint =3D 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) =3D=3D
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=C3=B8rn 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 =3D kzalloc(sizeof(struct s2255_dev), GFP_KERNEL);
> ..
>         for (i =3D 0; i < iface_desc->desc.bNumEndpoints; ++i) {
>                 endpoint =3D &iface_desc->endpoint[i].desc;
>                 if (!dev->read_endpoint && usb_endpoint_is_bulk_in(endpoi=
nt)) {
>                         /* we found the bulk in endpoint */
>                         dev->read_endpoint =3D 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=C3=B8rn


--=20
Best regards!

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