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

[Eugeniu Rosca]

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.

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

> > > +
> > > +       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:

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

Thanks again and looking forward to your feedback.

Best regards,
Eugeniu.