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

[Alex Kiernan]

On Wed, Mar 27, 2019 at 12:32 PM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:
>
> Hi Alex,
>
> Thanks for the precious comments. Some remarks below.
>
> On Wed, Mar 27, 2019 at 05:55:51AM +0000, Alex Kiernan wrote:
> > 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:
> [..]
> > > > 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.
>
> Applying below two-liner instrumentation on top of my patch:
>
> diff --git a/drivers/fastboot/fb_getvar.c b/drivers/fastboot/fb_getvar.c
> index c703be4cd61e..0149824c0d3d 100644
> --- a/drivers/fastboot/fb_getvar.c
> +++ b/drivers/fastboot/fb_getvar.c
> @@ -138,7 +138,9 @@ static void getvar_has_slot(char *part_name, char *response)
>                 return;
>
>         strlcpy(name, part_name, sizeof(name) - 2);
> +       printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
>         strcat(name, "_a");
> +       printf("%s: name: %s, strlen(name) %lu\n", __func__, name, strlen(name));
>
>         if (fastboot_mmc_get_part_info(name, &dev_desc, &info, response) >= 0)
>                 fastboot_okay("yes", response);
>
> Passing a 32+ character part-name to 'getvar has-slot' results in:
> $ host> fastboot getvar has-slot:0123456789abcdef0123456789abcdef0123456789abcdef
> $ target>
> getvar_has_slot: name: 0123456789abcdef0123456789abc, strlen(name) 29
> getvar_has_slot: name: 0123456789abcdef0123456789abc_a, strlen(name) 31
>
> From the above results, it looks to me that the partition name handling
> (including deliberate string truncation done by strlcpy) works
> correctly. I am still ready to rework/optimize the implementation if
> you have any concerns/counter-proposals.
>

Looking at the rest of the code, there's confusion as to whether it's
expecting a +1 or not, given disk_partition.name[] is PART_NAME_LEN, I
think what you have is right.

> >
> > > Also we should use PART_NAME_LEN not 32 (fb_mmc.c needs
> > > fixing in the same way).
>
> Agreed. Will be fixed in v2.
>
> > > > +
> > > > +       if (!part_name || !strcmp(part_name, ""))
> > > > +               return;
> > >
> > > This needs to do fastboot_okay/fastboot_fail or you'll get the
> > > protocol out of sync
>
> The idea was to avoid bloating U-Boot with string literals, but I can
> add a fastboot_fail() call if you wish so. IMO if the lack of
> fastboot_okay/fastboot_fail at the end of dispatching a fastboot call
> triggers undefined behavior on host side, then long-term this should
> be fixed in the U-Boot fastboot dispatcher itself. FWIW, the current
> behavior on my target is:
>
> host $> fastboot getvar has-slot
> getvar:has-slot FAILED (status malformed (0 bytes))
> Finished. Total time: 0.039s
> host $> fastboot getvar has-slot:
> getvar:has-slot: FAILED (status malformed (0 bytes))
> Finished. Total time: 0.039s
>

"status malformed" is a pretty poor failure. The other thing is check
both the UDP and USB paths - the UDP path gets stuck in timeouts if
you don't send something which is within the protocol (though probably
not for this case).

I'd agree that the structure is awkward - if you're looking at
implementing `getvar all` then I expect it all have to be refactored
as my recollection is for multiple values you have to send them as
INFO frames.

> > > > +
> > > > +       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.
>
> Agreed. Has to be fixed in v2. Do you have any preference between
> the build-time "#if CONFIG_IS_ENABLED()" and the run-time "IS_ENABLED()"
> which will be needed for NAND-specific handling? It is my feeling that
> in Linux realm the run-time checks are greatly preferred. Here is some
> feedback from Stephen Rothwell in https://lkml.org/lkml/2018/2/22/406:
>

I guess my preference is for consistency within a subsystem;
personally I prefer the IS_ENABLED() style, but I suspect there's some
refactoring needed to make that work.

> -----8<-----
> > I like IS_ENABLED() being used wherever
> > possible because it allows us better compiler coverage (in the face
> > of CONFIG options) even if the compiler then elides the actual code.
> > It also breaks the code up less than #ifdef's.
> -----8<-----
>
> > > Also fastboot_mmc_get_part_info() sends its own fastboot_fail so this
> > > just isn't going to work in a failure case.
>
> I agree that, with the patch applied, getvar_has_slot() will potentially
> override the response returned by fastboot_mmc_get_part_info(), but is
> this really a problem? Technically, this doesn't look like a problem,
> because we expect the 'strlcpy(response, tag, FASTBOOT_RESPONSE_LEN)'
> call from fastboot_response() to successfully overwrite any
> previously-stored response. The only problematic aspect is of somewhat
> increased complexity, since we allow the higher-level fastboot layers
> to redefine the status returned by the lower-level fastboot layers (hey,
> this sounds quite naturally to me after all). This is something related
> to internal implementation detail and I believe it is up to us to allow
> or forbid/discourage this. Whichever is our preference, IMO this goes
> beyond the scope of this patch. Calling fastboot_okay/fail currently
> happens non-uniformly across several fastboot internal components/files,
> so cleaning this up will require non-trivial amount of time.
>

I don't think we should be knowingly leaving traps for the unwary - if
the structure needs fixing, it needs fixing properly.

When I pulled in the UDP code, what started out as a trivial import
some external code turned into a wholesale refactor and reorganise of
the fastboot code so that there was just one application layer with
two underlying transports.

--
Alex Kiernan