[edk2-devel] [PATCH v2 2/3] UefiPayloadPkg: Add PayloadLoaderPeim which can load ELF payload
Ni, Ray
ray.ni at intel.com
Tue Jun 8 03:10:38 UTC 2021
Marvin,
Comments below.
> > +EFI_STATUS
> > +ProcessRelocation32 (
> > + IN Elf32_Rela *Rela,
> > + IN UINT32 RelaSize,
> > + IN UINT32 RelaEntrySize,
> > + IN UINT32 RelaType,
> > + IN INTN Delta,
> > + IN BOOLEAN DynamicLinking
> > + )
> > +{
> > + UINTN Index;
> > + UINT32 *Ptr;
> > + UINT32 Type;
> > +
> > + for ( Index = 0
> > + ; RelaEntrySize * Index < RelaSize
>
> Overflow?
>
Will change from:
RelaEntrySize * Index < RelaSize
to:
Index < RelaSize / RelaEntrySize
> > + ; Index++, Rela = ELF_NEXT_ENTRY (Elf32_Rela, Rela, RelaEntrySize)
> > + ) {
> > + //
> > + // r_offset is the virtual address of the storage unit affected by the relocation.
> > + //
> > + Ptr = (UINT32 *)(UINTN)(Rela->r_offset + Delta);
>
> Alignment?
>
I don't understand. Can you explain a bit more?
> > + if (DynamicLinking) {
> > + //
> > + // A: Represents the addend used to compute the value of the relocatable field.
> > + // B: Represents the base address at which a shared object has been loaded into memory during execution.
> > + // Generally, a shared object is built with a 0 base virtual address, but the execution address will be different.
> > + //
> > + // B (Base Address) in ELF spec is slightly different:
> > + // An executable or shared object file's base address (on platforms that support the concept) is calculated during
> > + // execution from three values: the virtual memory load address, the maximum page size, and the lowest virtual
> address
> > + // of a program's loadable segment. To compute the base address, one determines the memory address associated
> with the
> > + // lowest p_vaddr value for a PT_LOAD segment. This address is truncated to the nearest multiple of the maximum
> page size.
> > + // The corresponding p_vaddr value itself is also truncated to the nearest multiple of the maximum page size.
> > + //
> > + // *** The base address is the difference between the truncated memory address and the truncated p_vaddr value.
> ***
> > + //
> > + // Delta in this function is B.
> > + //
> > + // Calculation: B + A
> > + //
> > + if (RelaType == SHT_RELA) {
> > + ASSERT (*Ptr == 0);
> > + *Ptr = (UINT32) Delta + Rela->r_addend;
> > + } else {
> > + //
> > + // A is stored in the field of relocation for REL type.
> > + //
> > + *Ptr = (UINT32) Delta + *Ptr;
> > + }
> > + } else {
> > + //
> > + // non-Dynamic section doesn't contain entries of this type.
> > + //
> > + DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));
> > + ASSERT (FALSE);
> > + }
> > + break;
> > +
> > + default:
> > + DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Type));
> > + }
> > + }
>
> Out of pure interest, if performance is a concern, have you profiled
> this code vs one with two loops and "DynamicLinking" pulled out?
>
I don't think the performance is a concern here.
> > + //
> > + // It's abnormal a DYN ELF doesn't contain a dynamic section.
> > + //
> > + ASSERT (DynShdr != NULL);
> > + if (DynShdr == NULL) {
> > + return EFI_UNSUPPORTED;
> > + }
> > + ASSERT (DynShdr->sh_type == SHT_DYNAMIC);
> > + ASSERT (DynShdr->sh_entsize >= sizeof (*Dyn));
>
> Abnormalities in unknown/untrusted data must be filtered with a runtime
> check, not with an ASSERT.
>
Sure. I will add if-check below the assertion so assertion-enabled build can
report the errors earlier.
> > + for ( Index = 0, Dyn = (Elf32_Dyn *) (ElfCt->FileBase + DynShdr->sh_offset)
> > + ; Index < DynShdr->sh_size / DynShdr->sh_entsize
>
> Is "sh_entsize" checked for 0?
>
No need. Because code above makes sure sh_entsize >= sizeof (*Dyn).
> > + ASSERT (RelShdr->sh_type == RelaType);
> > + ASSERT (RelShdr->sh_entsize == RelaEntrySize);
>
> See above.
>
Agree. Will add if-checks.
> > + DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic sections...\n"));
> > + Status = RelocateElf32Dynamic (ElfCt);
> > + ASSERT_EFI_ERROR (Status);
>
> Why cannot this fail?
>
A DYN type ELF image should contain dynamic section. So only an abnormal ELF image can fail.
> > + return (Elf64_Phdr *)(ImageBase + Ehdr->e_phoff + Index * Ehdr->e_phentsize);
>
> Alignment checks? Bounds checks?
>
For the alignment checks, do you suggest that I should make sure the segment should be placed
in the address that meets the alignment requirement?
ELF spec just requires below for Elf64_Phdr.p_align:
loadable process segments must have congruent values for p_vaddr and p_offset, modulo the page size.
I can add such check in ParseElfImage().
> > + ProcessRelocation64 (
> > + (Elf64_Rela *) (ElfCt->FileBase + RelShdr->sh_offset),
>
> Alignment? :) I know there is no real concept in EDK II yet, but it
> really is needed.
>
Can you explain a bit more on the alignment?
> > +
> > +/**
> > + Check if the ELF image is valid.
> > +
> > + @param[in] ImageBase Memory address of an image.
> > +
> > + @retval TRUE if valid.
> > +
> > +**/
> > +BOOLEAN
> > +IsElfFormat (
> > + IN CONST UINT8 *ImageBase
>
> You cannot safely inspect untrusted/unknown data without a size field,
> also needs checks below.
>
Agree. Original idea was to add a ELF loader that can load the ELF assuming
the ELF image is well-formatted.
But with your help, I am glad to enhance the logic a bit more to expand
the support of external ELF images.
Will add a "UINTN ImageSize" parameter.
> > + )
> > +{
> > + Elf32_Ehdr *Elf32Hdr;
> > + Elf64_Ehdr *Elf64Hdr;
> > +
> > + ASSERT (ImageBase != NULL);
> > +
> > + Elf32Hdr = (Elf32_Ehdr *)ImageBase;
> > +
> > + //
> > + // Start with correct signature "\7fELF"
> > + //
> > + if ((Elf32Hdr->e_ident[EI_MAG0] != ELFMAG0) ||
> > + (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
> > + (Elf32Hdr->e_ident[EI_MAG1] != ELFMAG1) ||
> > + (Elf32Hdr->e_ident[EI_MAG2] != ELFMAG2)
> > + ) {
> > + return FALSE;
> > + }
> > +
> > + //
> > + // Support little-endian only
> > + //
> > + if (Elf32Hdr->e_ident[EI_DATA] != ELFDATA2LSB) {
> > + return FALSE;
> > + }
> > +
> > + //
> > + // Check 32/64-bit architecture
> > + //
> > + if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS64) {
> > + Elf64Hdr = (Elf64_Ehdr *)Elf32Hdr;
> > + Elf32Hdr = NULL;
> > + } else if (Elf32Hdr->e_ident[EI_CLASS] == ELFCLASS32) {
> > + Elf64Hdr = NULL;
> > + } else {
> > + return FALSE;
> > + }
>
> Why are the branches above and below separated when they map basically 1:1?
>
Indeed. Thanks for catching this.
Will merge the separate "if" together.
> > +
> > + if (Elf64Hdr != NULL) {
> > + //
> > + // Support intel architecture only for now
> > + //
> > + if (Elf64Hdr->e_machine != EM_X86_64) {
> > + return FALSE;
> > + }
> > +
> > + // Use last section as end of file
> > + Status = GetElfSectionPos (ElfCt, ElfCt->ShNum - 1, &Offset, &Size);
>
> What if ShNum is 0?
>
Agree. The logic to calculate file size might not be needed.
Let me confirm and try to remove the entire function.
> > + if (ElfCt->EiClass == ELFCLASS32) {
> > + Elf32Hdr = (Elf32_Ehdr *)ElfCt->FileBase;
> > + FileSize2 = Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hdr->e_shnum;
> > + } else if (ElfCt->EiClass == ELFCLASS64) {
> > + Elf64Hdr = (Elf64_Ehdr *)ElfCt->FileBase;
> > + FileSize2 = (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize * Elf64Hdr->e_shnum);
> > + }
>
> Overflows?
>
Integer overflow?
Will add integer overflow check if this file size calculation logic is still needed.
> > +
> > + if (ElfCt == NULL) {
> > + return EFI_INVALID_PARAMETER;
> > + }
>
> As this is function contract, I'd replace this with an ASSERT, or at
> least have both.
>
I will put "ASSERT (ElfCt != NULL)" above the if.
> > + ZeroMem (ElfCt, sizeof(ELF_IMAGE_CONTEXT));
> > +
> > + if (ImageBase == NULL) {
> > + return (ElfCt->ParseStatus = EFI_INVALID_PARAMETER);
>
> If I see it correctly, all instances that can assign ParseStatus also
> return it. Why is the member needed at all?
>
I expect that caller needs to call ParseElfImage() to get the ParseStatus
properly assigned before calling LoadElfImage().
The member ParseStatus is checked in LoadElfImage() later.
Today it's just PayloadLoaderPeim that calls the ElfLib functions.
But I expect that the ElfLib functions can be public lib APIs in future
if needs appear.
> > + Name = (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shdr->sh_name);
>
> 0-termination checks, or return size?
>
I will validate the string section in ParseElfImage(). The validation logic will:
1. Verify that each section name is pointed from the e_shstrndx
2. Verify that section name strings don't occupy spaces outside of the string section.
> > +
> > + ZeroMem (&Context, sizeof (Context));
>
> This is done by the callee already.
>
Indeed. Will remove this.
> > + Status = ParseElfImage (Elf, &Context);
> > + } while (EFI_ERROR (Status));
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76178): https://edk2.groups.io/g/devel/message/76178
Mute This Topic: https://groups.io/mt/83277976/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list