linux-kernel - Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 12 Apr 2019 10:04:09 +0200
From:   Bjørn Mork <bjorn@...k.no>
To:     Young Xiao <92siuyang@...il.com>
Cc:     kbuild-all@...org, linux-usb@...r.kernel.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        greg@...ah.com, mchehab@...nel.org, keescook@...omium.org,
        hans.verkuil@...co.com, Young Xiao <YangX92@...mail.com>
Subject: Re: [PATCH] USB: s2255 & stkwebcam: fix oops with malicious USB descriptors

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@...il.com> writes:

> From: Young Xiao <YangX92@...mail.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