Re: [edk2-devel] [PATCH 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: 5041 bytes --]

[AMD Official Use Only - General]



From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, November 7, 2023 1:55 AM
To: devel@edk2.groups.io; quic_llindhol@quicinc.com; ardb+tianocore@kernel.org; Chang, Abner <Abner.Chang@amd.com>; git@danielschaefer.me; Jeff Brasen <jbrasen@nvidia.com>
Subject: Re: [PATCH 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.

Hello,

Hoping to get some feedback on these 2 patches this week.

Thanks
Ashish
________________________________
From: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>
Sent: Monday, October 30, 2023 2:24 PM
To: 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>>; abner.chang@amd.com<mailto:abner.chang@amd.com> <abner.chang@amd.com<mailto:abner.chang@amd.com>>; git@danielschaefer.me<mailto:git@danielschaefer.me> <git@danielschaefer.me<mailto:git@danielschaefer.me>>; Jeff Brasen <jbrasen@nvidia.com<mailto:jbrasen@nvidia.com>>
Cc: Ashish Singhal <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>
Subject: [PATCH 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug

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 <ashishsingha@nvidia.com<mailto:ashishsingha@nvidia.com>>
---
 .../AndroidBootImgLib/AndroidBootImgLib.c     | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index 1359a66db2..02769cd0df 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -321,8 +321,9 @@ AndroidBootImgGetFdt (

 EFI_STATUS
 AndroidBootImgUpdateArgs (
-  IN  VOID  *BootImg,
-  OUT VOID  *KernelArgs
+  IN  VOID    *BootImg,
+  OUT VOID    *KernelArgs,
+  IN  UINT32  KernelArgsSize
   )
 {
   CHAR8       ImageKernelArgs[ANDROID_BOOTIMG_KERNEL_ARGS_SIZE];
@@ -337,13 +338,13 @@ AndroidBootImgUpdateArgs (
   AsciiStrToUnicodeStrS (
     ImageKernelArgs,
     KernelArgs,
-    ANDROID_BOOTIMG_KERNEL_ARGS_SIZE >> 1
+    KernelArgsSize
     );
   // Append platform kernel arguments
   if (mAndroidBootImg->AppendArgs) {
     Status = mAndroidBootImg->AppendArgs (
                                 KernelArgs,
-                                ANDROID_BOOTIMG_KERNEL_ARGS_SIZE
+                                KernelArgsSize
                                 );
   }

@@ -611,11 +612,16 @@ AndroidBootImgBoot (
   MEMORY_DEVICE_PATH         KernelDevicePath;
   EFI_HANDLE                 ImageHandle;
   VOID                       *NewKernelArg;
+  UINT32                     NewKernelArgSize;
   EFI_LOADED_IMAGE_PROTOCOL  *ImageInfo;
   VOID                       *RamdiskData;
   UINTN                      RamdiskSize;
   IN  VOID                   *FdtBase;

+  if ((Buffer == NULL) || (BufferSize == 0)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   NewKernelArg = NULL;
   ImageHandle  = NULL;

@@ -637,14 +643,15 @@ AndroidBootImgBoot (
     goto Exit;
   }

-  NewKernelArg = AllocateZeroPool (ANDROID_BOOTIMG_KERNEL_ARGS_SIZE);
+  NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
+  NewKernelArg     = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
I think you can just move the memory allocation code to inside AndroidBootImgUpdateArgs then you don't need the additional arg for AndroidBootImgUpdateArgs.
Also Change AndroidBootImgUpdateArgs (Buffer, NewKernelArg); to AndroidBootImgUpdateArgs (Buffer, &NewKernelArg);
With this the code looks simple.
thanks
Abner

   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, NewKernelArgSize);
   if (EFI_ERROR (Status)) {
     goto Exit;
   }
--
2.17.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111006): https://edk2.groups.io/g/devel/message/111006
Mute This Topic: https://groups.io/mt/102284239/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: 12809 bytes --]