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

[Marek Vasut]

On 6/20/23 11:17, Xavier Drudis Ferran wrote:
> 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.

That's fine

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

Default, see:
$ git grep CONFIG_BOOTCOMMAND configs/

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

So, what is the minimal test case ?
I have been asking for that for a while.

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

I would really like a minimal test case. Empty your env and figure out 
the commands which need to be executed to trigger this. Without any 
interference from other commands/scripting/...

> 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

Here you assume distro bootcommand or some such . Can we remove that 
assumption ? (I think yes, and we should)

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

Does it crash if you empty your env and run simply

=> usb reset ; bootflow scan ; usb info

?