[SPAM] Re: [PATCH] cmd: usb: Prevent reset in usb tree/info command

[Simon Glass]

Hi Xavier,

On Wed, 14 Jun 2023 at 09:40, Xavier Drudis Ferran <xdrudis at tinet.cat> wrote:
>
>
> Thanks for explaining, Simon Glass.
>
> Can someone please stop me if I'm splitting hairs or bikeshedding or
> something ? I guess we agree both checking for null or UCLASS_BOOTDEV
> would work right now but we are looking for what's more future-proof.
>
> El Tue, Jun 13, 2023 at 09:12:30PM +0100, Simon Glass deia:
> > Hi Xavier,
> >
> > On Tue, 13 Jun 2023 at 17:04, Xavier Drudis Ferran <xdrudis at tinet.cat> wrote:
> > >
> > > El Tue, Jun 13, 2023 at 03:58:22PM +0100, Simon Glass deia:
> > > >
> > > > Yes that's right. So 'usb info' should ignore UCLASS_BOOTDEV devices.
> > >
> > > That's a possibility, yes. It should work until someone adds another
> > > device there for some other purpose.
> > >
> > > > That is better than checking for the NULL pointer.
> > > >
> > >
> > > Why? What's wrong about checking for null ?
> > > Or maybe checking for both not null and  not UCLASS_BOOTDEV ?
> > >
> > > Not that I care too much, just to understand your reasoning.
> >
> > Well, devices may have something attached there, so it may be
> > non-NULL, but point to a different struct from the expected one.
> >
>
> Yes. That is possible. How likely ?
>
> That "there" is dev_get_parent_priv(). If I understand the driver
> model, those devices shouldn't put things there themselves, it should
> be their parent who puts stuff there for them. And the parent should
> be an usb_device->dev (because of the recursive code). So what other
> struct would such a parent put in a child parent_priv_ ? Yes, whatever
> it wants, but I mean, isn't it likely to assume the child would be
> "adopted" with null as parent_priv_ or a "natural child" with a usb_device
> in parent_priv_ ?
>
> I did a very rough search
>
> grep -rIE 'UCLASS_(BLK|BOOTDEV|USB_EMUL)' . |grep -F .id |cut -f1 -d:  |xargs -n 1 grep -l per_child_auto
> ./drivers/usb/emul/usb-emul-uclass.c
>
> Which seems to suggest only UCLASS_USB_EMUL would have parent_priv_,
> and that would be an usb_device, which the current code does not want
> to recurse anyway. (dev_set_parent_priv() is almost never called).
>
> It is also possible that one day a device that is not UCLASS_BLK,
> UCLASS_BOOTDEV or UCLASS_USB_EMUL is put as children of a usb storage
> device (just imagine a future system similar to bootstd for firmware
> updates, trust material, etc.). Is it likely to have a struct in
> parent_priv_ that is not a usb_device ?
>
> So which is more likely to survive future changes ?
>
> - checking for parent_priv_ not null and not UCLASS_USB_EMUL
>
> - checking for parent_priv_ not null and not UCLASS_USB_EMUL and not UCLASS_BLK
>   (my patch, overcautious ?)
>
> - checking for not (UCLASS_BLK, UCLASS_BOOTDEV or UCLASS_USB_EMUL)
>   (Simon Glass' idea)
>
> - checking for not  UCLASS_BLK and not UCLASS_BOOTDEV and not UCLASS_USB_EMUL
>            and parent_priv_ not null

Really the parent_priv thing is a separate issue, a side effect of
something having a UCLASS_USB parent.

The key point here is that we cannot iterate down into a bootdev
device looking for USB devices. So we should use that as the check,
since it is the most direct check.

>
> > >
> > > Or can we check for recursible devices somehow ?
> > > Maybe flag them or something ?
> > >
> > > Why is better to state all devices are recursible except some UCLASSes
> > > (meaning they can have stuff that needs listed in usb info - or
> > > likewise usb tree -) instead of stating that some closed set are
> > > recursible ?
> > >
> >
> > For USB we can have lots of different types of devices as children, so
> > it would be a pain to enumerate them all. At least that's how I see
> > it.
> >
>
> We can have lots of devices as children, but those do get listed
> before the recursive call.  How many of those can have children that
> we have to list too, i.e.  how many of those do we want to recurse
> into ?
>
> I can only think of usb hubs (maybe some node for multifunction
> devices too ???).  Maybe I'm not understanding how U-Boot works with
> USB devices... but it looks like it would be enough to look for
> UCLASS_USB_HUB, then recursive call, else no recursion. I mean instead of
>
> cmd/usb.c : usb_show_tree_graph() :
>
>         if ((device_get_uclass_id(  child  ) != UCLASS_USB_EMUL) &&
>             (device_get_uclass_id(  child  ) != UCLASS_BOOTDEV) &&
>             (device_get_uclass_id(  child  ) != UCLASS_BLK)) {
>                         usb_show_tree_graph(udev, pre);
>                         pre[index] = 0;
>                 }
>
> we could simply have
>
>         if (device_get_uclass_id(  dev->dev  ) == UCLASS_USB_HUB) {
>                         usb_show_tree_graph(udev, pre);
>                         pre[index] = 0;
>                 }
>
> (or put the condition directly before the for loop)
>
> Or can we have an UCLASS_USB_EMUL, UCLASS_BLK or UCLASS_BOOTDEV as
> direct child of an UCLASS_USB_HUB ???
>
> Am I opening any cans of worms ?

>From my memory, I think you can check for a USB hub instead, but I'm
not completely sure.

I suggest adding a test for the command (see test/dm/acpi.c for an
example) since a test is the best way to ensure this doesn't happen
again.

Regards,
Simon