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

[Alex Kiernan]

On Wed, Mar 27, 2019 at 5:44 AM Alex Kiernan <alex.kiernan at gmail.com> wrote:
>
> 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.

Sorry, wrong way around... can truncate the name by 1, not overflow.

> 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



-- 
Alex Kiernan