Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, Nicolas Belin nbelin@baylibre.com wrote:

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") Signed-off-by: Nicolas Belin nbelin@baylibre.com

Reviewed-by: Mattijs Korpershoek mkorpershoek@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); }

-- 2.34.1

Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, Nicolas Belin nbelin@baylibre.com wrote:

Free newbootargs when the concatenation is done and bootargs env is set.

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

-- 2.34.1

Hi Nicolas,

Thank you for the patch.

On mer., déc. 11, 2024 at 14:53, 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

---

boot/image-android.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c index 362a5c7435a3a8bcf7b674b96e31069a91a892b5..ed72a5c30424a453c1800bc61edbe8f33b31b341 100644 --- a/boot/image-android.c +++ b/boot/image-android.c @@ -289,7 +289,7 @@ int android_image_get_kernel(const void *hdr, int len = 0; if (*img_data.kcmdline) { printf("Kernel command line: %s\n", img_data.kcmdline);

• len += strlen(img_data.kcmdline);
  

• len += strlen(img_data.kcmdline) + 1; /* Extra space character needed */
  

  }

  if (*img_data.kcmdline_extra) {

@@ -299,28 +299,28 @@ int android_image_get_kernel(const void *hdr,

char *bootargs = env_get("bootargs"); if (bootargs)

• len += strlen(bootargs);
  

• len += strlen(bootargs) + 1; /* Extra space character needed */
  

In some cases, we will allocate for an extra space character when this is not needed.

For example, with: * bootargs = "b" * kcmdline = NULL * kcmdline_extra = NULL

We will allocate strlen("b") + 1 + 1. But we only need to allocate strlen("b") + 1.

Another example: * bootargs = "b" * kcmdline = "k" * kcmdline_extra = NULL

Will allocate strlen("b") + 1 + strlen("k") + 1 + 1. But we only need: strlen("b") + 1 + strlen("k") + 1.

How about we count the amount of elements that are not NULL to compute the amount of spaces: * 0 -> 0 space * 1 -> 0 space * 2 -> 1 space * 3 -> 2 spaces

• 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 */

The comment should state Null, not NULL. NULL is the pointer and Null is the \0 character.

if (bootargs) { strcpy(newbootargs, bootargs); strcat(newbootargs, " ");

A similar thing can be stated here. Sometimes, we add spaces which are not needed.

Can we rework this a little to only add what is needed?

}

• if (*img_data.kcmdline)

• if (*img_data.kcmdline) { strcat(newbootargs, img_data.kcmdline);

• if (*img_data.kcmdline_extra) { strcat(newbootargs, " ");

• strcat(newbootargs, img_data.kcmdline_extra);
  

  }

• if (*img_data.kcmdline_extra)

• strcat(newbootargs, img_data.kcmdline_extra);
  

• env_set("bootargs", newbootargs); free(newbootargs);

-- 2.34.1