Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

["Chang, Abner via groups.io" <abner.chang=amd.com@groups.io>]

[-- Attachment #1: Type: text/plain, Size: 12682 bytes --]

[AMD Official Use Only - General]

We need a PR to trigger CI test and then maintainer can set the label to "Push" to merge the code if the change passes all CI tests.  I think I done this way for a long time.

Regards,
Abner

From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Thursday, November 30, 2023 10:16 AM
To: Chang, Abner <Abner.Chang@amd.com>; devel@edk2.groups.io; quic_llindhol@quicinc.com; ardb+tianocore@kernel.org; git@danielschaefer.me; Jeff Brasen <jbrasen@nvidia.com>
Subject: Re: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug


[AMD Official Use Only - General]

Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.

I thought the process was to share the patch via email and maintainers create the PR. Author creating PR used to be optional. Has that changed recently?

Thanks
Ashish

Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>
Sent: Wednesday, November 29, 2023 7:13:24 PM
To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com> <quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>>; ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>; git@danielschaefer.me<mailto:git@danielschaefer.me> <git@danielschaefer.me<mailto:git@danielschaefer.me>>; Jeff Brasen <jbrasen@nvidia.com<mailto:jbrasen@nvidia.com>>
Subject: RE: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

External email: Use caution opening links or attachments


[AMD Official Use Only - General]


Ah sure.  I can do it at anytime. Could you please create a PR for this change and let me know?



Abner



From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>
Sent: Thursday, November 30, 2023 12:37 AM
To: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>; git@danielschaefer.me<mailto:git@danielschaefer.me>; Jeff Brasen <jbrasen@nvidia.com<mailto:jbrasen@nvidia.com>>
Subject: Re: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug



[AMD Official Use Only - General]



Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.



Hello,



Checking if there is a timeline for this patchset to be merged.



Thanks

Ashish



________________________________

From: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>
Sent: Monday, November 20, 2023 6:57 PM
To: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>>; quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com> <quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>>; ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org> <ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>>; git@danielschaefer.me<mailto:git@danielschaefer.me> <git@danielschaefer.me<mailto:git@danielschaefer.me>>; Jeff Brasen <jbrasen@nvidia.com<mailto:jbrasen@nvidia.com>>
Subject: RE: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug



External email: Use caution opening links or attachments


[AMD Official Use Only - General]

>I did not add this intentionally and even sent out an email asking the same on edk2-rfc if this should be done by maintainers unless you want me to do it as a ??separate patch set.
>Link to the message is https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Frfc%2Fmessage%2F827&data=05%7C01%7Cashishsingha%40nvidia.com%7C772572ba91f04cdfdf0108dbea352cbb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638361286344293715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wWhH%2BkdVGcCCs5uAWz0mwJ9ROrZ6OZ0lWkdHHnXcaMc%3D&reserved=0<https://edk2.groups.io/g/rfc/message/827>
That's fine.

> String comes in from the https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FEmbeddedPkg%2FInclude%2FLibrary%2FAndroidBootImgLib.h%23L43&data=05%7C01%7Cashishsingha%40nvidia.com%7C772572ba91f04cdfdf0108dbea352cbb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638361286344293715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=QZqBxMHLxQRlMNWVAqGTOq1pwtsQf6LYQG8UwTkEjmE%3D&reserved=0<https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Include/Library/AndroidBootImgLib.h#L43> header so it should ideally be self-contained. If you still think we should check for it, I can add a separate patch while keeping this one unaffected.

Reviewed-by: Abner Chang <abner.chang@amd.com<mailto:abner.chang@amd.com>>

Abner


From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>
Sent: Tuesday, November 21, 2023 3:34 AM
To: Chang, Abner <Abner.Chang@amd.com<mailto:Abner.Chang@amd.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; quic_llindhol@quicinc.com<mailto:quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org<mailto:ardb+tianocore@kernel.org>; git@danielschaefer.me<mailto:git@danielschaefer.me>; Jeff Brasen <jbrasen@nvidia.com<mailto:jbrasen@nvidia.com>>
Subject: Re: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

[AMD Official Use Only - General]


Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.



________________________________________
From: Chang, Abner <mailto:Abner.Chang@amd.com>
Sent: Monday, November 20, 2023 5:13 AM
To: Ashish Singhal <mailto:ashishsingha@nvidia.com>; mailto:devel@edk2.groups.io <mailto:devel@edk2.groups.io>; mailto:quic_llindhol@quicinc.com <mailto:quic_llindhol@quicinc.com>; mailto:ardb+tianocore@kernel.org <mailto:ardb+tianocore@kernel.org>; mailto:git@danielschaefer.me <mailto:git@danielschaefer.me>; Jeff Brasen <mailto:jbrasen@nvidia.com>
Subject: RE: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

External email: Use caution opening links or attachments


[AMD Official Use Only - General]

Thanks for this update and two more comments,

> -----Original Message-----
> From: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: mailto:devel@edk2.groups.io; mailto:quic_llindhol@quicinc.com;
> mailto:ardb+tianocore@kernel.org; Chang, Abner <mailto:Abner.Chang@amd.com>;
> mailto:git@danielschaefer.me; mailto:jbrasen@nvidia.com
> Cc: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> Subject: [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line
> Length Bug
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> Curently, AndroidBootImgLib expects input kernel command line
> to never exceed 256 unicode characters where the image header
> allows for 512 ascii characters. If image header allows 512
> ascii characters, similar number of unicode characters should be
> allowed at the minimum.
>
> Signed-off-by: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> ---
>  .../AndroidBootImgLib/AndroidBootImgLib.c     | 31 +++++++++++--------
>  1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index 1359a66db2..f63648e60d 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -322,11 +322,12 @@ AndroidBootImgGetFdt (
>  EFI_STATUS
>  AndroidBootImgUpdateArgs (
>    IN  VOID  *BootImg,
> -  OUT VOID  *KernelArgs
> +  OUT VOID  **KernelArgs
>    )
I know the original code doesn't have the function header. Would you mind to add it as we change the prototype of this function?
Or you probably would like to keep it as it for the consistence, as no function header for any of functions in this file.

I did not add this intentionally and even sent out an email asking the same on edk2-rfc if this should be done by maintainers unless you want me to do it as a separate patch set.

Link to the message is https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Frfc%2Fmessage%2F827&data=05%7C01%7Cashishsingha%40nvidia.com%7C772572ba91f04cdfdf0108dbea352cbb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638361286344293715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wWhH%2BkdVGcCCs5uAWz0mwJ9ROrZ6OZ0lWkdHHnXcaMc%3D&reserved=0<https://edk2.groups.io/g/rfc/message/827>


>  {
>    CHAR8       ImageKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
>    EFI_STATUS  Status;
> +  UINT32      NewKernelArgSize;
>
>    // Get kernel arguments from Android boot image
>    Status = AndroidBootImgGetKernelArgs (BootImg, ImageKernelArgs);
> @@ -334,16 +335,23 @@ AndroidBootImgUpdateArgs (
>      return Status;
>    }
>
Do you think we have to check if the ASCII string size in ImageKernelArgs greater than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE?

Thanks
Abner

String comes in from the https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fblob%2Fmaster%2FEmbeddedPkg%2FInclude%2FLibrary%2FAndroidBootImgLib.h%23L43&data=05%7C01%7Cashishsingha%40nvidia.com%7C772572ba91f04cdfdf0108dbea352cbb%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C638361286344293715%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=QZqBxMHLxQRlMNWVAqGTOq1pwtsQf6LYQG8UwTkEjmE%3D&reserved=0<https://github.com/tianocore/edk2/blob/master/EmbeddedPkg/Include/Library/AndroidBootImgLib.h#L43> header so it should ideally be self-contained. If you still think we should check for it, I can add a separate patch while keeping this one unaffected.

Thanks
Ashish


> +  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
> +  *KernelArgs      = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
> +  if (*KernelArgs == NULL) {
> +    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
>    AsciiStrToUnicodeStrS (
>      ImageKernelArgs,
> -    KernelArgs,
> -    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1
> +    *KernelArgs,
> +    NewKernelArgSize
>      );
>    // Append platform kernel arguments
>    if (mAndroidBootImg->AppendArgs) {
>      Status = mAndroidBootImg->AppendArgs (
> -                                KernelArgs,
> -                                ANDROID_BOOTIMG_KERNEL_ARGS_SIZE
> +                                *KernelArgs,
> +                                NewKernelArgSize
>                                  );
>    }
>
> @@ -616,6 +624,10 @@ AndroidBootImgBoot (
>    UINTN                      RamdiskSize;
>    IN  VOID                   *FdtBase;
>
> +  if ((Buffer == NULL) || (BufferSize == 0)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    NewKernelArg = NULL;
>    ImageHandle  = NULL;
>
> @@ -637,14 +649,7 @@ AndroidBootImgBoot (
>      goto Exit;
>    }
>
> -  NewKernelArg = AllocateZeroPool
> (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
> -  if (NewKernelArg == NULL) {
> -    DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> -    Status = EFI_OUT_OF_RESOURCES;
> -    goto Exit;
> -  }
> -
> -  Status = AndroidBootImgUpdateArgs (Buffer, NewKernelArg);
> +  Status = AndroidBootImgUpdateArgs (Buffer, &NewKernelArg);
>    if (EFI_ERROR (Status)) {
>      goto Exit;
>    }
> --
> 2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111881): https://edk2.groups.io/g/devel/message/111881
Mute This Topic: https://groups.io/mt/102598724/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 25292 bytes --]