Re: [PATCH] media: uvc: Avoid cyclic entity chains due to malformed USB descriptors

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Will,

On Wed, Oct 02, 2019 at 02:19:29PM +0100, Will Deacon wrote:
> 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?

Right. We can reject the whole chain in that case. There's one case
where we still want to succeed though, which is handled by
uvc_scan_fallback().

Looking at the code, uvc_scan_device() does

                if (uvc_scan_chain(chain, term) < 0) {
                        kfree(chain);
                        continue;
                }

It seems that's missing removal of all entities that would have been
successfully added to the chain. This prevents, I think,
uvc_scan_fallback() from working properly in some cases.

> > 		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?

Oops. This is embarassing... :-) You're of course right. The second hunk
seems fine too, even if I would have preferred centralising the check in
a single place. That should be possible, but it would involve
refactoring that isn't worth it at the moment.

-- 
Regards,

Laurent Pinchart