Hi Alexey, On mer., avril 03, 2024 at 08:49, Alexey Romanov <avroma...@salutedevices.com> wrote:
> Hello Mattijs, > is there any feedback? Sorry for the late reply. I was both swamped with other work and awaiting. feedback from others. I don't have strong enough arguments to state that this is not useful to others, I have re-considered this and I'm willing to pick it up. Please rebase, as this no longer applies. Also see some review comments below > > On Thu, Feb 15, 2024 at 10:14:13AM +0100, Mattijs Korpershoek wrote: >> Hi Alexey, >> >> Thank you for the patch. >> >> On jeu., f'evr. 01, 2024 at 12:20, Alexey Romanov >> <avroma...@salutedevices.com> wrote: >> >> > Currently, fastboot protocol in U-Boot has no opportunity >> > to execute vendor custom code with verifed boot. This patch >> > introduce new fastboot subcommand fastboot oem board:<cmd>, >> > which allow to run custom oem_board function. >> > >> > Default implementation is __weak. Vendor must redefine it in >> > board/ folder with his own logic. >> > >> > For example, some vendors have their custom nand/emmc partition >> > flashing or erasing. Here some typical command for such use cases: >> > >> > - flashing: >> > >> > $ fastboot stage bootloader.img >> > $ fastboot oem board:write_bootloader >> > >> > - erasing: >> > >> > $ fastboot oem board:erase_env >> > >> > Signed-off-by: Alexey Romanov <avroma...@salutedevices.com> >> >> Sorry for the delay. I needed time to give this some thoughts and I >> waited for Sean to chime as well on this. >> >> I've heard from Neil that this might be related to: >> https://github.com/superna9999/pyamlboot/pull/20 >> >> I think this can be useful. Not necessarily for writing custom >> partitions, but I see this could be used for other things: >> >> 1. Provision SoC-specific fuses (serialno/mac addr) at the factory line >> (for production devices) >> Examples: >> $ fastboot oem board:write_serialno ABCDEF >> $ fastboot oem board:write_macaddr AA:BB:CC:DD:EE >> >> 2. Access secure storage (via an Trusted Application) >> >> But both examples could also be in a fastboot flash format: >> $ fastboot flash serialno ABCDEF >> >> One concern I have is that U-Boot forks might use this command as >> an excuse to not makes things generic. >> >> I hope that others will chime in on this as well. >> I'd like to discuss this more because once this command is in we cannot >> remove it later. >> >> > --- >> > drivers/fastboot/Kconfig | 7 +++++++ >> > drivers/fastboot/fb_command.c | 15 +++++++++++++++ >> > include/fastboot.h | 1 + >> > 3 files changed, 23 insertions(+) >> > >> > diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig >> > index a4313d60a9..4d94391a76 100644 >> > --- a/drivers/fastboot/Kconfig >> > +++ b/drivers/fastboot/Kconfig >> > @@ -241,6 +241,13 @@ config FASTBOOT_OEM_RUN >> > this feature if you are using verified boot, as it will allow an >> > attacker to bypass any restrictions you have in place. >> > >> > +config FASTBOOT_OEM_BOARD >> > + bool "Enable the 'oem board' command" >> > + help >> > + This extends the fastboot protocol with an "oem board" command. This >> > + command allows running vendor custom code defined in board/ files. >> > + Otherwise, it will do nothing and send fastboot fail. >> >> If we move forward with this, please also document the new command in: >> doc/android/fastboot.rst This still applies, document the command please. >> >> > + >> > endif # FASTBOOT >> > >> > endmenu >> > diff --git a/drivers/fastboot/fb_command.c b/drivers/fastboot/fb_command.c >> > index 5fcadcdf50..2298815770 100644 >> > --- a/drivers/fastboot/fb_command.c >> > +++ b/drivers/fastboot/fb_command.c >> > @@ -40,6 +40,7 @@ static void reboot_recovery(char *, char *); >> > static void oem_format(char *, char *); >> > static void oem_partconf(char *, char *); >> > static void oem_bootbus(char *, char *); >> > +static void oem_board(char *, char *); >> > static void run_ucmd(char *, char *); >> > static void run_acmd(char *, char *); >> > >> > @@ -107,6 +108,10 @@ static const struct { >> > .command = "oem run", >> > .dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_RUN, (run_ucmd), >> > (NULL)) >> > }, >> > + [FASTBOOT_COMMAND_OEM_BOARD] = { >> > + .command = "oem board", >> > + .dispatch = CONFIG_IS_ENABLED(FASTBOOT_OEM_BOARD, (oem_board), >> > (NULL)) >> > + }, >> > [FASTBOOT_COMMAND_UCMD] = { >> > .command = "UCmd", >> > .dispatch = CONFIG_IS_ENABLED(FASTBOOT_UUU_SUPPORT, (run_ucmd), >> > (NULL)) >> > @@ -490,3 +495,13 @@ static void __maybe_unused oem_bootbus(char >> > *cmd_parameter, char *response) >> > else >> > fastboot_okay(NULL, response); >> > } >> > + >> > +void __weak fastboot_oem_board(char *cmd_parameter, void *data, u32 size, >> > char *response) >> > +{ >> > + fastboot_fail("oem board function not defined", response); >> > +} >> > + >> > +static void __maybe_unused oem_board(char *cmd_parameter, char *response) >> > +{ >> > + fastboot_oem_board(cmd_parameter, fastboot_buf_addr, image_size, >> > response); >> > +} Please also document the functions with comment headers, as done for the other oem_ functions. >> > diff --git a/include/fastboot.h b/include/fastboot.h >> > index 296451f89d..06c1f26b6c 100644 >> > --- a/include/fastboot.h >> > +++ b/include/fastboot.h >> > @@ -37,6 +37,7 @@ enum { >> > FASTBOOT_COMMAND_OEM_PARTCONF, >> > FASTBOOT_COMMAND_OEM_BOOTBUS, >> > FASTBOOT_COMMAND_OEM_RUN, >> > + FASTBOOT_COMMAND_OEM_BOARD, >> > FASTBOOT_COMMAND_ACMD, >> > FASTBOOT_COMMAND_UCMD, >> > FASTBOOT_COMMAND_COUNT >> > -- >> > 2.30.1 > > -- > Thank you, > Alexey
