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

[Xavier Drudis Ferran]

El Mon, Jun 19, 2023 at 11:49:18PM +0200, Marek Vasut deia:
> On 6/19/23 12:12, Xavier Drudis Ferran wrote:
> 
> It seems the email addresses are being constantly corrupted in each email.
> This time the ML address is wrong and missing an e at the end. There is some
> e@ nonexistent address which I have to keep removing.
>

Yes, that's my fault. I'm sorry. I apologize to you and others.  I
resent my mail with the proper address. Please just look for my mail
with the wrong address and delete it from your mail archive to prevent
further such problems. You can reply to the other mail I sent (June
19th), because it has the same content, just with an added
apology. Sorry again.

> > When DISTRO_DEFAULTS is not set, the default environment has
> > bootcmd=bootflow
> 
> That is not right, on $randomboard I picked the bootcmd is something else.
>

And how default is your default environment for your $randomboard ?

Almost half the configs/* redefine CONFIG_BOOTCOMMAND (524/1268)

When DISTRO_DEFAULTS is not set, that makes BOOTSTD_BOOTCOMMAND default to y
and the default for BOOTCMD is not "run distro_boootcmd", but "bootflow scan"
or (for sandbox) "bootflow scan -lb". When there's bootcmd at all.

But this is only the default for the default environment. It can be
overriden and the Kconfig is not exactly simple. An extract:

next branch:

arch/Arm/Kconfig:

config ARCH_ROCKCHIP
	[...]
	imply BOOTSTD_DEFAULTS
	[...]


cmd/Kconfig:

config CMD_BOOTFLOW
        bool "bootflow"
        depends on BOOTSTD
        default y
	[...]

config CMD_BOOTFLOW_FULL
        bool "bootflow - extract subcommands"
        depends on BOOTSTD_FULL
        default y
	[...]

boot/Kconfig:

config BOOT_DEFAULTS
        bool  # Common defaults for standard boot and distroboot
        imply USE_BOOTCOMMAND
	[...]

config BOOTSTD
        bool "Standard boot support"
        default y
	[...]

config BOOTSTD_FULL
        bool "Enhanced features for standard boot"
        default y if SANDBOX
        [...]


if BOOTSTD
[...]

config BOOTSTD_DEFAULTS
        bool "Select some common defaults for standard boot"
        depends on BOOTSTD
        imply USE_BOOTCOMMAND
        select BOOT_DEFAULTS
        select BOOTMETH_DISTRO
	[...]

config BOOTSTD_BOOTCOMMAND
        bool "Use bootstd to boot"
        default y if !DISTRO_DEFAULTS
	[...]
[...]
endif
[...]
config DISTRO_DEFAULTS
        bool "Select defaults suitable for booting general purpose Linux distributions"
        select BOOT_DEFAULTS
	[...]

config BOOTCOMMAND
        string "bootcmd value"
        depends on USE_BOOTCOMMAND && !USE_DEFAULT_ENV_FILE
        default "bootflow scan -lb" if BOOTSTD_DEFAULTS && CMD_BOOTFLOW_FULL
        default "bootflow scan" if BOOTSTD_DEFAULTS && !CMD_BOOTFLOW_FULL
        default "run distro_bootcmd" if !BOOTSTD_BOOTCOMMAND && DISTRO_DEFAULTS
> 
> Does this happen if you set empty bootcmd ('=> setenv bootcmd 'echo hello'
> for example), then 'saveenv' , then 'reset' , then drop into U-Boot shell
> and run 'usb reset ; usb info' too ?
>

I haven't tested it. If bootflow scan is not run it might not happen.
Someone has to hang the UCLASS_BOOTDEV on the usb mass storage device,
for it to fail. But as far as I know the idea is to make bootflow the
default in more and more cases. You'll always be able to avoid it
running in your board by setting your own environment at runtime or
changing the configuration, yes, but what's the point ?

I thought that failing one scenario was enough to fix things.  When
one finds a bug it tries to help others to reproduce it.  When others
help the bug finder to run other scenarios that don't have the bug,
what's that useful for ?

Note that it won't fail if the boot succeeds, because then you won't
have a shell to run usb info/tree. It won't fail if usb is not in
boot_targets. It won't fail if there's no mass storage device
connected to usb when bootflow scan is run...

But I still think the failing case is worth fixing. Someone may be
wondering why bootflow fails, run usb info and find a reset, when
setting up a new board, or trying to boot from the wrong usb stick
after the system partition has been corrupted, or whatever. It's not
something that breaks any board in production, but it's not something
to leave forever broken. In theory a null pointer dereference might be
used by some attacker, but in this case I don't really see any useful
attack, maybe it's my lack of imagination. So I'm not claiming it's a
severe bug. It's just a normal bug that needs fixing when possible.

Or are you trying to hint that the solution shouldn't be changing
cmd/usb.c but cleaning up the UCLASS_BOOTDEVs after bootflow scan
somehow ?

Or I should change the commit message because the point is not so much
what's the default environment or the default default environment, but
simply that bootflow scan is run with an usb mass storage device
connected and no boot content present in any of the boot_targets
media, and then usb tree/info is run ?

If it's just that you can't reproduce it, can you try to ?:

- set up a board with no boot media (I tested like this but it might
  not be needed),

- put usb in boot_targets (if you put only usb there you may not need
  the previous step):
  setenv boot_targets usb  

- plug a non-booting usb mass storage device to an usb port,

- run usb reset in case you already had usb initialized at boot, or
  skip this if usb is not initialized yet. If in doubt, run usb reset.

- run bootflow scan

- run usb info

It should list some devices, but give you a reset just after listing the
usb mass storage device without my patch, and it should just list all
usb devices and go back to the prompt with my patch.