Re: [PATCH v2 1/5] boot/image-android: Workaround kernel/ramdisk invalid addr

[Mattijs Korpershoek <mkorpershoek@kernel.org>]

Hi George,

On jeu., mai 08, 2025 at 12:22, george chan <gchan9527@gmail.com> wrote:

> Hi Mattijs,
>
> Thx for feedback.
>
> 在 2025年5月7日週三 15:47，Mattijs Korpershoek <mkorpershoek@kernel.org> 寫道：
>
>> Hi George,
>>
>> Thank you for the patch.
>> I really prefer this solution to the one proposed in v1.
>>
>> I have tested this on Khadas VIM3 board (using
>> khadas-vim3_android_defconfig)
>>
>> I have a couple of small remarks below.
>>
>> On lun., mai 05, 2025 at 17:17, George Chan via B4 Relay <
>> devnull+gchan9527.gmail.com@kernel.org> wrote:
>>
>> > From: George Chan <gchan9527@gmail.com>
>> >
>> > Some androidboot image have invalid kernel/ramdisk load addr,
>> > force to ignore those value and use loadaddr instead.
>>
>> Can we please elaborate a bit more in the commit message?
>> We have not explained/justified why repacking a boot image with proper
>> kernel ramdisk/load addr is not possible.
>>
>> Is the following an acceptable justification for you?
>> """
>> Since it's not always possible to change the load addr by repacking
>> the boot.img (due to AVB signature mismatch/<or_another_justification>),
>> we need a way to use kernel_addr_r and ramdisk_addr_r.
>> """
>>
>> Feel free to reformulate the above.
>>
>
> Thx. let me sum up as below 2 reasons.
>
> 1. there is a concern on exposing the whole memory to image loading is
> dangerous
>
> 2. Since it's not always possible to change the load addr by repacking
> the boot.img (mainly due to AVB signature mismatch),
> we need a way to use kernel_addr_r and ramdisk_addr_r.

That's good summary. Please include it in the commit message for v3.

>
>
>> >
>> > Suggested-by: Casey Connolly <casey.connolly@linaro.org>
>> > Signed-off-by: George Chan <gchan9527@gmail.com>
>> > ---
>> >  boot/Kconfig         | 6 ++++++
>> >  boot/image-android.c | 9 ++++++---
>> >  2 files changed, 12 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/boot/Kconfig b/boot/Kconfig
>> > index fb37d912bc9..4bdac384181 100644
>> > --- a/boot/Kconfig
>> > +++ b/boot/Kconfig
>> > @@ -11,6 +11,12 @@ config ANDROID_BOOT_IMAGE
>> >         This enables support for booting images which use the Android
>> >         image format header.
>> >
>> > +config ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR
>> > +     bool "Android Boot Image ignore addr"
>> > +     default n
>> > +     help
>> > +       This ignore kernel/ramdisk load addr specified in androidboot
>> header.
>>
>> Please add a bit more context here, checkpatch.pl reports issues:
>> $ ./scripts/checkpatch.pl --u-boot --git master..HEAD
>>
>> WARNING: please write a paragraph that describes the config symbol fully
>> #27: FILE: boot/Kconfig:14:
>> +config ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR
>>
>
> same as above.
>
>>
>> > +
>> >  config TIMESTAMP
>> >       bool "Show image date and time when displaying image information"
>> >       default y if CMD_DATE
>> > diff --git a/boot/image-android.c b/boot/image-android.c
>> > index 1746b018900..7b8eb6a4f64 100644
>> > --- a/boot/image-android.c
>> > +++ b/boot/image-android.c
>> > @@ -268,7 +268,8 @@ static ulong android_image_get_kernel_addr(struct
>> andr_image_data *img_data,
>> >        *
>> >        * Otherwise, we will return the actual value set by the user.
>> >        */
>> > -     if (img_data->kernel_addr  == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR) {
>> > +     if (img_data->kernel_addr  == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR ||
>> > +             IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR)) {
>>
>> Please fix indentation, should be:
>>
>> +       if (img_data->kernel_addr  == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR ||
>> +           IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR)) {
>>
>> (checkpatch.pl reports this)
>>
>
> ack.
>
>
>> >               if (comp == IH_COMP_NONE)
>> >                       return img_data->kernel_ptr;
>> >               return env_get_ulong("kernel_addr_r", 16, 0);
>> > @@ -464,7 +465,8 @@ int android_image_get_ramdisk(const void *hdr, const
>> void *vendor_boot_img,
>> >        */
>> >       if (img_data.header_version > 2) {
>> >               /* Ramdisk can't be used in-place, copy it to
>> ramdisk_addr_r */
>> > -             if (img_data.ramdisk_addr ==
>> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
>> > +             if (img_data.ramdisk_addr ==
>> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR ||
>> > +
>>  (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR))) {
>>
>> Same here. Also, can we please drop the leading parenthesis (IS_ENABLED(
>> -> IS_ENABLED( to be consistent with the previous code change?
>>
>
> ack.
>
>
>> >                       ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16,
>> 0);
>> >                       if (!ramdisk_ptr) {
>> >                               printf("Invalid ramdisk_addr_r to copy
>> ramdisk into\n");
>> > @@ -488,7 +490,8 @@ int android_image_get_ramdisk(const void *hdr, const
>> void *vendor_boot_img,
>> >       } else {
>> >               /* Ramdisk can be used in-place, use current ptr */
>> >               if (img_data.ramdisk_addr == 0 ||
>> > -                 img_data.ramdisk_addr ==
>> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
>> > +                 img_data.ramdisk_addr ==
>> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR ||
>> > +
>>  (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR))) {
>>
>> Same here. Also, can we please drop the leading parenthesis (IS_ENABLED(
>> -> IS_ENABLED( to be consistent with the previous code change?
>>
>
> ack.
>
> l planed to release v3 later around end of next week and contain only patch
> #1 #2 and #5 to ease maintainer processing. please feel free to comment on
> any idea, i will include into next version.
>
> regards,
> George
>
>>