[U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots

[Alex Kiernan]

On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> With eMMC partitioning [1], 'fastboot getvar has-slot:<part-name>'
> returns 'yes' only for 'system' and 'boot', while it is clear that [1]
> has more partitions featuring slots (i.e. dtb, dtbo, vbmeta and vendor).
>
> One not so obvious consequence is that
> 'fastboot flash {dtb,dtbo,vbmeta,vendor} img' will fail [2], while
> 'fastboot flash {boot,system} img' will succeed [3]. This is because
> 'fastboot flash' relies on the outcome of
> 'fastboot has-slot:<part-name>' behind the scene.
>
> Assuming that the list of partitions featuring slots is vendor-
> specific, U-Boot fastboot driver is not the best place for such list.
>
> To avoid creating __weak functions overridden by board code, one simple
> solution seems to be taking the user-provided partition as base, append
> the "_a" suffix and checking if the result is a valid partition name.
>
> This approach assumes that the 'has-slot:' API is specifically
> designed to work with partition names chopped of their slot suffixes.
> Reviewing the usage of 'has-slot' in upstream fastboot [4], this
> assumption seems to be fortified, but to be 100% sure we need an Ack
> from a fastboot expert.
>
> [1] R-Car-H3 eMMC partitioning used for testing:
> (bootloader)  Created new GPT partition table:
> (bootloader)      /misc (512 KiB, raw)
> (bootloader)      /pst (512 KiB, raw)
> (bootloader)      /vbmeta_a (512 KiB, raw)
> (bootloader)      /vbmeta_b (512 KiB, raw)
> (bootloader)      /dtb_a (1024 KiB, raw)
> (bootloader)      /dtb_b (1024 KiB, raw)
> (bootloader)      /dtbo_a (512 KiB, raw)
> (bootloader)      /dtbo_b (512 KiB, raw)
> (bootloader)      /boot_a (16384 KiB, raw)
> (bootloader)      /boot_b (16384 KiB, raw)
> (bootloader)      /metadata (16384 KiB, raw)
> (bootloader)      /system_a (2301952 KiB, ext4)
> (bootloader)      /system_b (2301952 KiB, ext4)
> (bootloader)      /vendor_a (262144 KiB, ext4)
> (bootloader)      /vendor_b (262144 KiB, ext4)
> (bootloader)      /userdata (2451951 KiB, ext4)
>
> [2] fastboot flash vbmeta vbmeta.img
> target reported max download size of 16777216 bytes
> Sending 'vbmeta' (4 KB)...
> OKAY [  0.025s]
> Writing 'vbmeta'...
> FAILED (remote: cannot find partition)
> Finished. Total time: 0.332s
>
> [3] fastboot flash boot boot.img
> target reported max download size of 16777216 bytes
> Sending 'boot_a' (16384 KB)...
> OKAY [  1.586s]
> Writing 'boot_a'...
> OKAY [  0.379s]
> Finished. Total time: 2.054s
>
> [4] core (f959fffc1c8c) git grep -l has-slot
> fastboot/constants.h
> fastboot/fastboot.cpp
> fastboot/fuzzy_fastboot/main.cpp
> fs_mgr/tests/adb-remount-test.sh
>
> Fixes: f73a7df984a9 ("net: fastboot: Merge AOSP UDP fastboot")
> Signed-off-by: Eugeniu Rosca <erosca at de.adit-jv.com>
> ---
>  drivers/fastboot/fb_getvar.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index 4d264c985d7e..03bcd7162b37 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -130,8 +130,17 @@ static void getvar_slot_suffixes(char *var_parameter, char *response)
>
>  static void getvar_has_slot(char *part_name, char *response)
>  {
> -       if (part_name && (!strcmp(part_name, "boot") ||
> -                         !strcmp(part_name, "system")))
> +       struct blk_desc *dev_desc;
> +       disk_partition_t info;
> +       char name[32];

For the code as written this needs to be 33, or the strcat can
overflow by 1. Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
fixing in the same way).

> +
> +       if (!part_name || !strcmp(part_name, ""))
> +               return;

This needs to do fastboot_okay/fastboot_fail or you'll get the
protocol out of sync

> +
> +       strlcpy(name, part_name, sizeof(name) - 2);
> +       strcat(name, "_a");
> +
> +       if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
>                 fastboot_okay("yes", response);
>         else
>                 fastboot_okay("no", response);

This will fail if you're building with CONFIG_FASTBOOT_FLASH_NAND.
Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
just isn't going to work in a failure case.

> --
> 2.21.0
>


-- 
Alex Kiernan