[U-Boot] [PATCH] fastboot: getvar: fix broken 'fastboot flash' when partition has slots
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@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]; + + if (!part_name || !strcmp(part_name, "")) + return; + + 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);
On Wed, Mar 27, 2019 at 1:04 AM Eugeniu Rosca erosca@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@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. 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
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:
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@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
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.
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.
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
Hi Alex,
Thanks yet again for the timely and insightful replies. I'll be sending out a v2 tackling your review findings as soon as I can.
Best regards, Eugeniu.
participants (2)
-
Alex Kiernan -
Eugeniu Rosca