* [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
@ 2023-11-15 3:12 Ashish Singhal via groups.io
2023-11-15 3:12 ` [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line Ashish Singhal via groups.io
2023-11-20 12:13 ` [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Chang, Abner via groups.io
0 siblings, 2 replies; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-15 3:12 UTC (permalink / raw)
To: devel, quic_llindhol, ardb+tianocore, abner.chang, git, jbrasen
Cc: Ashish Singhal
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>
---
.../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
)
{
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;
}
+ 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 (#111229): https://edk2.groups.io/g/devel/message/111229
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line
2023-11-15 3:12 [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Ashish Singhal via groups.io
@ 2023-11-15 3:12 ` Ashish Singhal via groups.io
2023-11-20 12:30 ` Chang, Abner via groups.io
2023-11-20 12:13 ` [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Chang, Abner via groups.io
1 sibling, 1 reply; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-15 3:12 UTC (permalink / raw)
To: devel, quic_llindhol, ardb+tianocore, abner.chang, git, jbrasen
Cc: Ashish Singhal
AndroidBootImgLib allows for platforms to append to kernel command
line but does not allow for the overall kernel command line to go
beyond the limit set by the image header. Address this limitation
by adding a pcd where platform can tell how many extra characters
they expect on their platform in addition to what the image header
specifies.
Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
---
EmbeddedPkg/EmbeddedPkg.dec | 5 +++++
EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 2 +-
EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 341ef5e6a6..94dc3c9b76 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -183,3 +183,8 @@
# Selection between DT and ACPI as a default
#
gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x0000059
+
+ #
+ # Expected Overflow Android Kernel Command Line Characters
+ #
+ gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UINT32|0x000005C
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index f63648e60d..d16929f2bb 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
return Status;
}
- NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
+ NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32 (PcdAndroidKernelCommandLineOverflow);
*KernelArgs = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
if (*KernelArgs == NULL) {
DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
index 8eefeef4f9..9754664df5 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
@@ -45,5 +45,6 @@
gEfiAcpiTableGuid
gFdtTableGuid
-[FeaturePcd]
+[Pcd]
gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
+ gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111228): https://edk2.groups.io/g/devel/message/111228
Mute This Topic: https://groups.io/mt/102598723/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-15 3:12 [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Ashish Singhal via groups.io
2023-11-15 3:12 ` [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line Ashish Singhal via groups.io
@ 2023-11-20 12:13 ` Chang, Abner via groups.io
2023-11-20 19:33 ` Ashish Singhal via groups.io
1 sibling, 1 reply; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-20 12:13 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me,
jbrasen@nvidia.com
[AMD Official Use Only - General]
Thanks for this update and two more comments,
> -----Original Message-----
> From: Ashish Singhal <ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Chang, Abner <Abner.Chang@amd.com>;
> git@danielschaefer.me; jbrasen@nvidia.com
> Cc: Ashish Singhal <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 <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.
> {
> 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
> + 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 (#111467): https://edk2.groups.io/g/devel/message/111467
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line
2023-11-15 3:12 ` [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line Ashish Singhal via groups.io
@ 2023-11-20 12:30 ` Chang, Abner via groups.io
2023-11-20 17:18 ` Ashish Singhal via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-20 12:30 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me,
jbrasen@nvidia.com
[AMD Official Use Only - General]
> -----Original Message-----
> From: Ashish Singhal <ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Chang, Abner <Abner.Chang@amd.com>;
> git@danielschaefer.me; jbrasen@nvidia.com
> Cc: Ashish Singhal <ashishsingha@nvidia.com>
> Subject: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command
> line
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> AndroidBootImgLib allows for platforms to append to kernel command
> line but does not allow for the overall kernel command line to go
> beyond the limit set by the image header. Address this limitation
> by adding a pcd where platform can tell how many extra characters
> they expect on their platform in addition to what the image header
> specifies.
>
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
> EmbeddedPkg/EmbeddedPkg.dec | 5 +++++
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 2 +-
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec
> b/EmbeddedPkg/EmbeddedPkg.dec
> index 341ef5e6a6..94dc3c9b76 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -183,3 +183,8 @@
> # Selection between DT and ACPI as a default
> #
> gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x0000059
> +
> + #
> + # Expected Overflow Android Kernel Command Line Characters
> + #
> +
> gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UI
> NT32|0x000005C
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index f63648e60d..d16929f2bb 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
> return Status;
> }
>
> - NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
> + NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32
> (PcdAndroidKernelCommandLineOverflow);
I don't know the history of ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, however, I am a little bit confused.
Is this PCD introduced for the case the string size may greater than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE? If yes, then why don't we just use StrSize (ImageKernelArgs) * sizeof(UINT16) as the buffer size?
Regards,
Abner
> *KernelArgs = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
> if (*KernelArgs == NULL) {
> DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index 8eefeef4f9..9754664df5 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -45,5 +45,6 @@
> gEfiAcpiTableGuid
> gFdtTableGuid
>
> -[FeaturePcd]
> +[Pcd]
> gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
> + gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
> --
> 2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111470): https://edk2.groups.io/g/devel/message/111470
Mute This Topic: https://groups.io/mt/102598723/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line
2023-11-20 12:30 ` Chang, Abner via groups.io
@ 2023-11-20 17:18 ` Ashish Singhal via groups.io
2023-11-21 1:52 ` Chang, Abner via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-20 17:18 UTC (permalink / raw)
To: Chang, Abner, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- Attachment #1: Type: text/plain, Size: 4899 bytes --]
________________________________
From: Chang, Abner <Abner.Chang@amd.com>
Sent: Monday, November 20, 2023 5:30 AM
To: Ashish Singhal <ashishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; git@danielschaefer.me <git@danielschaefer.me>; Jeff Brasen <jbrasen@nvidia.com>
Subject: RE: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line
External email: Use caution opening links or attachments
[AMD Official Use Only - General]
> -----Original Message-----
> From: Ashish Singhal <ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Chang, Abner <Abner.Chang@amd.com>;
> git@danielschaefer.me; jbrasen@nvidia.com
> Cc: Ashish Singhal <ashishsingha@nvidia.com>
> Subject: [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command
> line
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> AndroidBootImgLib allows for platforms to append to kernel command
> line but does not allow for the overall kernel command line to go
> beyond the limit set by the image header. Address this limitation
> by adding a pcd where platform can tell how many extra characters
> they expect on their platform in addition to what the image header
> specifies.
>
> Signed-off-by: Ashish Singhal <ashishsingha@nvidia.com>
> ---
> EmbeddedPkg/EmbeddedPkg.dec | 5 +++++
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 2 +-
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec
> b/EmbeddedPkg/EmbeddedPkg.dec
> index 341ef5e6a6..94dc3c9b76 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -183,3 +183,8 @@
> # Selection between DT and ACPI as a default
> #
> gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x0000059
> +
> + #
> + # Expected Overflow Android Kernel Command Line Characters
> + #
> +
> gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UI
> NT32|0x000005C
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index f63648e60d..d16929f2bb 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
> return Status;
> }
>
> - NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
> + NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32
> (PcdAndroidKernelCommandLineOverflow);
I don't know the history of ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, however, I am a little bit confused.
Is this PCD introduced for the case the string size may greater than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE? If yes, then why don't we just use StrSize (ImageKernelArgs) * sizeof(UINT16) as the buffer size?
Regards,
Abner
Android style image header can never have more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. However, a platform may need to add new kernel command line arguments at boot time that may need more buffer space than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. To allow for that, we can have a PCD that by default is set to 0 but a user can override that in their platform dsc if they expect more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE characters. This is not a bugfix but a limitation that is being addressed in this patch.
Thanks
Ashish
> *KernelArgs = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
> if (*KernelArgs == NULL) {
> DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index 8eefeef4f9..9754664df5 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -45,5 +45,6 @@
> gEfiAcpiTableGuid
> gFdtTableGuid
>
> -[FeaturePcd]
> +[Pcd]
> gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
> + gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
> --
> 2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111484): https://edk2.groups.io/g/devel/message/111484
Mute This Topic: https://groups.io/mt/102598723/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: 8656 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-20 12:13 ` [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Chang, Abner via groups.io
@ 2023-11-20 19:33 ` Ashish Singhal via groups.io
2023-11-21 1:57 ` Chang, Abner via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-20 19:33 UTC (permalink / raw)
To: Chang, Abner, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- Attachment #1: Type: text/plain, Size: 5535 bytes --]
________________________________
From: Chang, Abner <Abner.Chang@amd.com>
Sent: Monday, November 20, 2023 5:13 AM
To: Ashish Singhal <ashishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; git@danielschaefer.me <git@danielschaefer.me>; Jeff Brasen <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 <ashishsingha@nvidia.com>
> Sent: Wednesday, November 15, 2023 11:12 AM
> To: devel@edk2.groups.io; quic_llindhol@quicinc.com;
> ardb+tianocore@kernel.org; Chang, Abner <Abner.Chang@amd.com>;
> git@danielschaefer.me; jbrasen@nvidia.com
> Cc: Ashish Singhal <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 <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://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://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 (#111488): https://edk2.groups.io/g/devel/message/111488
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: 10509 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line
2023-11-20 17:18 ` Ashish Singhal via groups.io
@ 2023-11-21 1:52 ` Chang, Abner via groups.io
0 siblings, 0 replies; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-21 1:52 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[AMD Official Use Only - General]
Ok, I got it. Thanks.
Reviewed-by: Abner Chang <abner.chang@amd.com>
Abner
From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, November 21, 2023 1:18 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 2/2] EmbeddedPkg: Allow longer android kernel command line
[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:30 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 2/2] EmbeddedPkg: Allow longer android kernel command line
External email: Use caution opening links or attachments
[AMD Official Use Only - General]
> -----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 2/2] EmbeddedPkg: Allow longer android kernel command
> line
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> AndroidBootImgLib allows for platforms to append to kernel command
> line but does not allow for the overall kernel command line to go
> beyond the limit set by the image header. Address this limitation
> by adding a pcd where platform can tell how many extra characters
> they expect on their platform in addition to what the image header
> specifies.
>
> Signed-off-by: Ashish Singhal <mailto:ashishsingha@nvidia.com>
> ---
> EmbeddedPkg/EmbeddedPkg.dec | 5 +++++
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 2 +-
> EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf | 3 ++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec
> b/EmbeddedPkg/EmbeddedPkg.dec
> index 341ef5e6a6..94dc3c9b76 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -183,3 +183,8 @@
> # Selection between DT and ACPI as a default
> #
> gEmbeddedTokenSpaceGuid.PcdDefaultDtPref|TRUE|BOOLEAN|0x0000059
> +
> + #
> + # Expected Overflow Android Kernel Command Line Characters
> + #
> +
> gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow|0|UI
> NT32|0x000005C
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index f63648e60d..d16929f2bb 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -335,7 +335,7 @@ AndroidBootImgUpdateArgs (
> return Status;
> }
>
> - NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE;
> + NewKernelArgSize = ANDROID_BOOTIMG_KERNEL_ARGS_SIZE + PcdGet32
> (PcdAndroidKernelCommandLineOverflow);
I don't know the history of ANDROID_BOOTIMG_KERNEL_ARGS_SIZE, however, I am a little bit confused.
Is this PCD introduced for the case the string size may greater than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE? If yes, then why don't we just use StrSize (ImageKernelArgs) * sizeof(UINT16) as the buffer size?
Regards,
Abner
Android style image header can never have more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. However, a platform may need to add new kernel command line arguments at boot time that may need more buffer space than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE bytes. To allow for that, we can have a PCD that by default is set to 0 but a user can override that in their platform dsc if they expect more than ANDROID_BOOTIMG_KERNEL_ARGS_SIZE characters. This is not a bugfix but a limitation that is being addressed in this patch.
Thanks
Ashish
> *KernelArgs = AllocateZeroPool (sizeof (CHAR16) * NewKernelArgSize);
> if (*KernelArgs == NULL) {
> DEBUG ((DEBUG_ERROR, "Fail to allocate memory\n"));
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> index 8eefeef4f9..9754664df5 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.inf
> @@ -45,5 +45,6 @@
> gEfiAcpiTableGuid
> gFdtTableGuid
>
> -[FeaturePcd]
> +[Pcd]
> gEmbeddedTokenSpaceGuid.PcdAndroidBootLoadFile2
> + gEmbeddedTokenSpaceGuid.PcdAndroidKernelCommandLineOverflow
> --
> 2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111494): https://edk2.groups.io/g/devel/message/111494
Mute This Topic: https://groups.io/mt/102598723/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-20 19:33 ` Ashish Singhal via groups.io
@ 2023-11-21 1:57 ` Chang, Abner via groups.io
2023-11-29 16:37 ` Ashish Singhal via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-21 1:57 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[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://edk2.groups.io/g/rfc/message/827
That's fine.
> String comes in from the 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>
Abner
From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, November 21, 2023 3:34 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.
________________________________________
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://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://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 (#111496): https://edk2.groups.io/g/devel/message/111496
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-21 1:57 ` Chang, Abner via groups.io
@ 2023-11-29 16:37 ` Ashish Singhal via groups.io
2023-11-30 2:13 ` Chang, Abner via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-29 16:37 UTC (permalink / raw)
To: Chang, Abner, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- Attachment #1: Type: text/plain, Size: 9303 bytes --]
Hello,
Checking if there is a timeline for this patchset to be merged.
Thanks
Ashish
________________________________
From: Chang, Abner <Abner.Chang@amd.com>
Sent: Monday, November 20, 2023 6:57 PM
To: Ashish Singhal <ashishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; git@danielschaefer.me <git@danielschaefer.me>; Jeff Brasen <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>
Abner
From: Ashish Singhal <ashishsingha@nvidia.com>
Sent: Tuesday, November 21, 2023 3:34 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.
________________________________________
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 (#111851): https://edk2.groups.io/g/devel/message/111851
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: 18517 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-29 16:37 ` Ashish Singhal via groups.io
@ 2023-11-30 2:13 ` Chang, Abner via groups.io
2023-11-30 2:15 ` Ashish Singhal via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-30 2:13 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- Attachment #1: Type: text/plain, Size: 10557 bytes --]
[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>
Sent: Thursday, November 30, 2023 12:37 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.
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 (#111879): https://edk2.groups.io/g/devel/message/111879
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: 19862 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-30 2:13 ` Chang, Abner via groups.io
@ 2023-11-30 2:15 ` Ashish Singhal via groups.io
2023-11-30 2:20 ` Chang, Abner via groups.io
0 siblings, 1 reply; 12+ messages in thread
From: Ashish Singhal via groups.io @ 2023-11-30 2:15 UTC (permalink / raw)
To: Chang, Abner, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- Attachment #1: Type: text/plain, Size: 11367 bytes --]
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>
Sent: Wednesday, November 29, 2023 7:13:24 PM
To: Ashish Singhal <ashishsingha@nvidia.com>; devel@edk2.groups.io <devel@edk2.groups.io>; quic_llindhol@quicinc.com <quic_llindhol@quicinc.com>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; git@danielschaefer.me <git@danielschaefer.me>; Jeff Brasen <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>
Sent: Thursday, November 30, 2023 12:37 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.
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 (#111880): https://edk2.groups.io/g/devel/message/111880
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: 20958 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug
2023-11-30 2:15 ` Ashish Singhal via groups.io
@ 2023-11-30 2:20 ` Chang, Abner via groups.io
0 siblings, 0 replies; 12+ messages in thread
From: Chang, Abner via groups.io @ 2023-11-30 2:20 UTC (permalink / raw)
To: Ashish Singhal, devel@edk2.groups.io, quic_llindhol@quicinc.com,
ardb+tianocore@kernel.org, git@danielschaefer.me, Jeff Brasen
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-11-30 2:20 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-15 3:12 [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Ashish Singhal via groups.io
2023-11-15 3:12 ` [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: Allow longer android kernel command line Ashish Singhal via groups.io
2023-11-20 12:30 ` Chang, Abner via groups.io
2023-11-20 17:18 ` Ashish Singhal via groups.io
2023-11-21 1:52 ` Chang, Abner via groups.io
2023-11-20 12:13 ` [edk2-devel] [PATCH v2 1/2] EmbeddedPkg: Fix Android Boot Command Line Length Bug Chang, Abner via groups.io
2023-11-20 19:33 ` Ashish Singhal via groups.io
2023-11-21 1:57 ` Chang, Abner via groups.io
2023-11-29 16:37 ` Ashish Singhal via groups.io
2023-11-30 2:13 ` Chang, Abner via groups.io
2023-11-30 2:15 ` Ashish Singhal via groups.io
2023-11-30 2:20 ` Chang, Abner via groups.io
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox