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:   Wed, 2 Oct 2019 14:19:29 +0100
From:   Will Deacon <will@...nel.org>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        andreyknvl@...gle.com, Mauro Carvalho Chehab <mchehab@...nel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>, stable@...r.kernel.org
Subject: Re: [PATCH] media: uvc: Avoid cyclic entity chains due to malformed
 USB descriptors

Hi Laurent,

On Wed, Oct 02, 2019 at 04:09:13PM +0300, Laurent Pinchart wrote:
> Thank you for the patch.

And thank you for the quick response.

> On Wed, Oct 02, 2019 at 12:27:53PM +0100, Will Deacon wrote:
> > I don't have a way to reproduce the original issue, so this change is
> > based purely on inspection. Considering I'm not familiar with USB nor
> > UVC, I may well have missed something!
> 
> I may also be missing something, I haven't touched this code for a long
> time :-)

Actually, that is pretty helpful because it will make backporting easier
if we get to that :)

> uvc_scan_chain_entity(), at the end of the function, adds the entity to
> the list of entities in the chain with
> 
> 	list_add_tail(&entity->chain, &chain->entities);

Yes.

> uvc_scan_chain_forward() is then called (from uvc_scan_chain()), and
> iterates over all entities connected to the entity being scanned.
> 
> 	while (1) {
> 		forward = uvc_entity_by_reference(chain->dev, entity->id,
> 			forward);

Yes.

> At this point forward may be equal to entity, if entity references
> itself.

Correct -- that's indicative of a malformed entity which we want to reject,
right?

> 		if (forward == NULL)
> 			break;
> 		if (forward == prev)
> 			continue;
> 		if (forward->chain.next || forward->chain.prev) {
> 			uvc_trace(UVC_TRACE_DESCR, "Found reference to "
> 				"entity %d already in chain.\n", forward->id);
> 			return -EINVAL;
> 		}
> 
> But then this check should trigger, as forward == entity and entity is
> in the chain's list of entities.

Right, but this code is added by my patch, no? In mainline, the code only
has the first two checks, which both end up comparing against NULL the first
time around:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/usb/uvc/uvc_driver.c#n1489

Or are you referring to somewhere else?

Will

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ