On Wed, Mar 27, 2019 at 12:32 PM Eugeniu Rosca erosca@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@gmail.com wrote:
On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca erosca@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