Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LF/CRLF is not coverted to NUL in Multiboot module string #1913

Closed
jiaqingz-intel opened this issue May 29, 2023 · 2 comments
Closed

LF/CRLF is not coverted to NUL in Multiboot module string #1913

jiaqingz-intel opened this issue May 29, 2023 · 2 comments

Comments

@jiaqingz-intel
Copy link
Contributor

When loading Multiboot container, the LF/CRLF is not coverted to NUL in Multiboot module string when booting

The command I used to build container is below, it was copied from https://github.com/intel/meta-acrn/blob/master/docs/slimbootloader.md

sudo python3 GenContainer.py create -cl CMDL:cmdline.txt ACRN:acrn.32.out CMD0:sos-cmdline.txt IMG0:vmlinuz -k ./SblKeys/OS1_TestKey_Priv_RSA3072.pem -a RSA3072_PKCS1_SHA2_384 -t MULTIBOOT -o container.bin

Both ACRN and SlimBootloader prints the string of the multiboot module as below, followed by chararcters not belonging to string

Linux_bzImage
  LZDMh, 

Multiboot Spec (https://www.gnu.org/software/grub/manual/multiboot/multiboot.html#Boot-information-format) says both cmdline and module string are null-terminated string. Is not converting to the NUL termintaor expected?

The cmdline is parsed at

//
// Allocate a cmd line buffer and init it with config file or default value
// Later, maybe there is new command line parameters appended
//
NewCmdBuffer = (UINT8 *) AllocatePages (EFI_SIZE_TO_PAGES (CMDLINE_LENGTH_MAX));
if (NewCmdBuffer == NULL) {
return EFI_OUT_OF_RESOURCES;
}
CmdFile = &LoadedImage->Image.Common.CmdFile;
Size = 0;
if (CmdFile->Addr != NULL && CmdFile->Size != 0) {
Size = (UINT32)GetFromConfigFile (NewCmdBuffer, CMDLINE_LENGTH_MAX, (UINT8 *)CmdFile->Addr, CmdFile->Size);
}
if (Size == 0) {
Size = (UINT32)AsciiStrSize (DEFAULT_COMMAND_LINE);
CopyMem (NewCmdBuffer, DEFAULT_COMMAND_LINE, Size);
}
// Free Allocated Memory earlier first
FreeImageData (CmdFile);
// Assign new one
CmdFile->Addr = NewCmdBuffer;
CmdFile->Size = Size;
CmdFile->AllocType = ImageAllocateTypePage;

stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 30, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string.

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
@stanleyintel
Copy link
Contributor

Hi Jiaqing,

Thanks for reporting the issue. A PR is created to fix it.

stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString() to load
mod string from a IMAGE_DATA (allocating a new buffer when requring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile() because the function
is designed to be compatible with leagcy format (e.g, EOF signature)
and truncate new line chars (which is not required by multiboot mod).

Minor change:
  - A typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because that function
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

Minor change:
  - A typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because that function
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

Minor change:
  - A typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because that function
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

FOr performance, the patch does not check "ASCII".

Minor change:
  - A typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because that function
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

For performance, the patch does not run "isascii" check.

Minor change:
  - A typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h
  - Dump mod string for debug build

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Signed-off-by: Stanley Chang <stanley.chang@intel.com>
stanleyintel added a commit to stanleyintel/slimbootloader that referenced this issue May 31, 2023
…der#1913

Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because GetFromConfigFile
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

For performance, the patch does not run "isascii" check.

Minor changes:
  - Fix typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h
  - Dump mod string for debug build

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Acked-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
Signed-off-by: Stanley Chang <stanley.chang@intel.com>
@stanleyintel
Copy link
Contributor

Hi,

As described in the PR, the patch fixes the issue that a mod string shall be a zero-terminated ASCII string (required by multboot spec), but it does not convert newlines to NULL (not required by multiboot spec).

gdong1 pushed a commit that referenced this issue Jun 7, 2023
Required by multiboot spec (*1), a mod string is a zero-terminated
ASCII string. The patch introduces LoadMultibootModString to load
mod string from a IMAGE_DATA (allocating a new buffer when requiring
to append zero-terminated char).

The patch does not reuse GetFromConfigFile because GetFromConfigFile
was designed to be compatible with legacy format (e.g., EOF signature)
and truncates newline chars (which is not required by multiboot mod).

For performance, the patch does not run "isascii" check.

Minor changes:
  - Fix typo error (InitMultibootMmap)
  - Declare FreeImageData in Library/IasImageLib.h
  - Dump mod string for debug build

Reference:
1. https://www.gnu.org/software/grub/manual/multiboot/multiboot.html

Verify: EHL CRB

Acked-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
Signed-off-by: Stanley Chang <stanley.chang@intel.com>
jiaqingz-intel added a commit to jiaqingz-intel/slimbootloader that referenced this issue Aug 17, 2023
LoadMultibootModString() checks the module index against MultiBoot->
MbModuleNumber, which value has not been assigned when called, causing
failure when loading multiboot image with modules. Remove the check to
fix it.

Fixes c9d70e7 ("fix: multiboot mod string in zero-terminated ASCII
format slimbootloader#1913")

Signed-off-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
gdong1 pushed a commit that referenced this issue Aug 17, 2023
LoadMultibootModString() checks the module index against MultiBoot->
MbModuleNumber, which value has not been assigned when called, causing
failure when loading multiboot image with modules. Remove the check to
fix it.

Fixes c9d70e7 ("fix: multiboot mod string in zero-terminated ASCII
format #1913")

Signed-off-by: Jiaqing Zhao <jiaqing.zhao@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants