[PATCH v2 0/3] boot: android: rework the bootargs concatenation
Rework the bootargs concatenation logic to make the logic clearer and address several issues such as: - checking the value at the address kcmdline_extra instead of checking the address value itself - freeing the string that was malloced for the concatenation - making sure to malloc the right amount of bytes for the concatenated string, not forgetting the byte for the NULL termination
Signed-off-by: Nicolas Belin nbelin@baylibre.com --- Changes in v2: - Improve the logic to avoid trailing whitespaces in the concatenated bootargs - Drop the last patch reordering the length calculation as it is already done by the improved logic - Link to v1: https://lore.kernel.org/r/20241211-fix-bootargs-concatenation-v1-0-c6752bcb9...
--- Nicolas Belin (3): boot: android: fix extra command line support boot: android: free newbootargs when done boot: android: rework bootargs concatenation
boot/image-android.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) --- base-commit: b841e559cd26ffcb20f22e8ee75debed9616c002 change-id: 20241211-fix-bootargs-concatenation-8b616c110b63
Best regards,
Check that the value at the address kcmdline_extra is not 0 instead of checking the address value itself keeping it consistent with what is done for kcmdline.
Fixes: b36b227b ("android: boot: support extra command line") Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com Signed-off-by: Nicolas Belin nbelin@baylibre.com --- boot/image-android.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/boot/image-android.c b/boot/image-android.c index cd01278f211d63262f2bdad7aa1176e2c1bbfedd..57158280b41c6552c82838e21384d925d5f7cde4 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -292,7 +292,7 @@ int android_image_get_kernel(const void *hdr, len += strlen(img_data.kcmdline); }
- if (img_data.kcmdline_extra) { + if (*img_data.kcmdline_extra) { printf("Kernel extra command line: %s\n", img_data.kcmdline_extra); len += strlen(img_data.kcmdline_extra); } @@ -316,7 +316,7 @@ int android_image_get_kernel(const void *hdr, if (*img_data.kcmdline) strcat(newbootargs, img_data.kcmdline);
- if (img_data.kcmdline_extra) { + if (*img_data.kcmdline_extra) { strcat(newbootargs, " "); strcat(newbootargs, img_data.kcmdline_extra); }
Free newbootargs when the concatenation is done and bootargs env is set.
Fixes: 86f4695b ("image: Fix Android boot image support") Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com Signed-off-by: Nicolas Belin nbelin@baylibre.com --- boot/image-android.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/boot/image-android.c b/boot/image-android.c index 57158280b41c6552c82838e21384d925d5f7cde4..362a5c7435a3a8bcf7b674b96e31069a91a892b5 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -322,6 +322,7 @@ int android_image_get_kernel(const void *hdr, }
env_set("bootargs", newbootargs); + free(newbootargs);
if (os_data) { if (image_get_magic(ihdr) == IH_MAGIC) {
Rework the bootargs concatenation allocating more accurately the length that is needed. Do not forget an extra byte for the null termination byte as, in some cases, the allocation was 1 byte short.
Fixes: 86f4695b ("image: Fix Android boot image support") Signed-off-by: Nicolas Belin nbelin@baylibre.com --- boot/image-android.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/boot/image-android.c b/boot/image-android.c index 362a5c7435a3a8bcf7b674b96e31069a91a892b5..61ac312db7ad9ba6c55727469dd4ded61824c67c 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -287,37 +287,40 @@ int android_image_get_kernel(const void *hdr, kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
int len = 0; + char *bootargs = env_get("bootargs"); + + if (bootargs) + len += strlen(bootargs); + if (*img_data.kcmdline) { printf("Kernel command line: %s\n", img_data.kcmdline); - len += strlen(img_data.kcmdline); + len += strlen(img_data.kcmdline) + (len ? 1 : 0); /* +1 for extra space */ }
if (*img_data.kcmdline_extra) { printf("Kernel extra command line: %s\n", img_data.kcmdline_extra); - len += strlen(img_data.kcmdline_extra); + len += strlen(img_data.kcmdline_extra) + (len ? 1 : 0); /* +1 for extra space */ }
- char *bootargs = env_get("bootargs"); - if (bootargs) - len += strlen(bootargs); - - char *newbootargs = malloc(len + 2); + char *newbootargs = malloc(len + 1); /* +1 for the '\0' */ if (!newbootargs) { puts("Error: malloc in android_image_get_kernel failed!\n"); return -ENOMEM; } - *newbootargs = '\0'; + *newbootargs = '\0'; /* set to Null in case no components below are present */
- if (bootargs) { + if (bootargs) strcpy(newbootargs, bootargs); - strcat(newbootargs, " "); - }
- if (*img_data.kcmdline) + if (*img_data.kcmdline) { + if (*newbootargs) /* If there is something in newbootargs, a space is needed */ + strcat(newbootargs, " "); strcat(newbootargs, img_data.kcmdline); + }
if (*img_data.kcmdline_extra) { - strcat(newbootargs, " "); + if (*newbootargs) /* If there is something in newbootargs, a space is needed */ + strcat(newbootargs, " "); strcat(newbootargs, img_data.kcmdline_extra); }
Hi Nicolas,
Thank you for the patch.
On mar., déc. 17, 2024 at 14:29, Nicolas Belin nbelin@baylibre.com wrote:
Rework the bootargs concatenation allocating more accurately the length that is needed. Do not forget an extra byte for the null termination byte as, in some cases, the allocation was 1 byte short.
Fixes: 86f4695b ("image: Fix Android boot image support") Signed-off-by: Nicolas Belin nbelin@baylibre.com
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
boot/image-android.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/boot/image-android.c b/boot/image-android.c index 362a5c7435a3a8bcf7b674b96e31069a91a892b5..61ac312db7ad9ba6c55727469dd4ded61824c67c 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -287,37 +287,40 @@ int android_image_get_kernel(const void *hdr, kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
int len = 0;
- char *bootargs = env_get("bootargs");
- if (bootargs)
len += strlen(bootargs);- if (*img_data.kcmdline) { printf("Kernel command line: %s\n", img_data.kcmdline);
len += strlen(img_data.kcmdline);
len += strlen(img_data.kcmdline) + (len ? 1 : 0); /* +1 for extra space */}
if (*img_data.kcmdline_extra) { printf("Kernel extra command line: %s\n", img_data.kcmdline_extra);
len += strlen(img_data.kcmdline_extra);
len += strlen(img_data.kcmdline_extra) + (len ? 1 : 0); /* +1 for extra space */
- char *bootargs = env_get("bootargs");
- if (bootargs)
len += strlen(bootargs);- char *newbootargs = malloc(len + 2);
- char *newbootargs = malloc(len + 1); /* +1 for the '\0' */ if (!newbootargs) { puts("Error: malloc in android_image_get_kernel failed!\n"); return -ENOMEM; }
- *newbootargs = '\0';
- *newbootargs = '\0'; /* set to Null in case no components below are present */
- if (bootargs) {
- if (bootargs) strcpy(newbootargs, bootargs);
strcat(newbootargs, " ");}
if (*img_data.kcmdline)
if (*img_data.kcmdline) {
if (*newbootargs) /* If there is something in newbootargs, a space is needed */strcat(newbootargs, " ");strcat(newbootargs, img_data.kcmdline);
}
if (*img_data.kcmdline_extra) {
strcat(newbootargs, " ");
if (*newbootargs) /* If there is something in newbootargs, a space is needed */strcat(newbootargs, " ");-- 2.34.1
Hi,
On Tue, 17 Dec 2024 14:29:07 +0100, Nicolas Belin wrote:
Rework the bootargs concatenation logic to make the logic clearer and address several issues such as:
- checking the value at the address kcmdline_extra instead of
checking the address value itself
- freeing the string that was malloced for the concatenation
- making sure to malloc the right amount of bytes for the concatenated
string, not forgetting the byte for the NULL termination
[...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/3] boot: android: fix extra command line support https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/53a0ddb6d3bed9f... [2/3] boot: android: free newbootargs when done https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/fd8b44a81be55b3... [3/3] boot: android: rework bootargs concatenation https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/9e5fad0f792539a...
-- Mattijs
participants (2)
-
Mattijs Korpershoek -
Nicolas Belin