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.kier...@gmail.com> wrote:
> > On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca <ero...@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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
