From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout01.posteo.de (mout01.posteo.de [185.67.36.65]) by mx.groups.io with SMTP id smtpd.web10.9546.1623139925283785337 for ; Tue, 08 Jun 2021 01:12:06 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=qIps1K50; spf=pass (domain: posteo.de, ip: 185.67.36.65, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout01.posteo.de (Postfix) with ESMTPS id 918F0240037 for ; Tue, 8 Jun 2021 10:12:03 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1623139923; bh=gJNJt3GcKWOqgQAN1eqotHZgXCvzbqqGC9yi+2QSYeg=; h=Subject:To:Cc:From:Date:From; b=qIps1K50r1Wu3/uLcRgTCbo54QlX0cS6xVwivMzFuFIUGE76FcIgsAmq/qNTWAk/j p3ajF/mwHj0AypVqhbWlb++ebiBOz76oLiaXPNNTQvmDeoGnCpCegqmosffYcKUOl7 ebUjCc7PdHKgbP+V1jbEkTNS69JUhzUwcii2YavnPM4yRXmiNnAG1RIprV9NSgV1RC D2SkwuvFoAr1dUhEDEDtm06xvfwm+iUYXb6QLe15IuoKlYNx0woGoQFKWjytVwdGeq WmptIN7qoSDadxgdOHLWSTZH3PjHnCuPEfF0CvVnbFnizhddkaJ/WLC0dnoBL4hZGR cSIv8rJ3phrcg== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4FzjdG1Skxz6tmF; Tue, 8 Jun 2021 10:12:01 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v2 2/3] UefiPayloadPkg: Add PayloadLoaderPeim which can load ELF payload To: devel@edk2.groups.io, ray.ni@intel.com Cc: "Ma, Maurice" , "Dong, Guo" , "You, Benjamin" References: <20210603062259.1390-1-ray.ni@intel.com> <20210603062259.1390-3-ray.ni@intel.com> <812b8f13-e951-5d27-9bd1-61711e6dd840@posteo.de> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <486c5ab8-240e-3ac5-5a4a-7f368cb68644@posteo.de> Date: Tue, 8 Jun 2021 08:12:01 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable Thank you for your quick reply, comments inline. On 08.06.21 05:10, Ni, Ray wrote: > 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 =3D 0 >>> + ; RelaEntrySize * Index < RelaSize >> Overflow? >> > Will change from: > RelaEntrySize * Index < RelaSize > to: > Index < RelaSize / RelaEntrySize imo add ASSERT for RelaEntrySize > 0 then. > > >>> + ; Index++, Rela =3D ELF_NEXT_ENTRY (Elf32_Rela, Rela, RelaEntry= Size) >>> + ) { >>> + // >>> + // r_offset is the virtual address of the storage unit affected b= y the relocation. >>> + // >>> + Ptr =3D (UINT32 *)(UINTN)(Rela->r_offset + Delta); >> Alignment? >> > I don't understand. Can you explain a bit more? Basically the alignment of any offset with which a pointer to=20 non-trivially-aligned (i.e. requirement > 1 Byte) data can be forged=20 must be checked. Examples from our new PE loader: https://github.com/mhaeuser/ISPRASOpen-SecurePE/tree/6a7abcd8647cf6faa7330= 82f6d8dcc9adc141d7e/src/PeCoffInit.c#L1226=20 ->=20 https://github.com/mhaeuser/ISPRASOpen-SecurePE/tree/6a7abcd8647cf6faa7330= 82f6d8dcc9adc141d7e/src/PeCoffInit.c#L1242 https://github.com/mhaeuser/ISPRASOpen-SecurePE/tree/6a7abcd8647cf6faa7330= 82f6d8dcc9adc141d7e/src/PeCoffInit.c#L1389=20 ->=20 https://github.com/mhaeuser/ISPRASOpen-SecurePE/tree/6a7abcd8647cf6faa7330= 82f6d8dcc9adc141d7e/src/PeCoffInit.c#L148 The idea here is that the base pointer (i.e. image header) is "maximally= =20 aligned" (i.e. can hold data of any platform data alignment). For the 8=20 Bytes AllocatePool() guarantees (file data), this is sufficient for any=20 primitive and composite data type. For the 4 KB AllocatePages()=20 guarantees (destination), this is sufficient of that, and for advanced=20 things like AVX (however not needed here). If the base is maximally=20 aligned, Base + X is guaranteed aligned for A if X is aligned for A,=20 i.e. X % _Alignof(A) =3D 0. Failing to verify this can cause exceptions on= = =20 platforms which don't support or have disabled the capability to perform= =20 unaligned memory accesses. > > >>> + if (DynamicLinking) { >>> + // >>> + // A: Represents the addend used to compute the value of th= e 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 vir= tual 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 a= ddress, the maximum page size, and the lowest virtual >> address >>> + // of a program's loadable segment. To compute the base a= ddress, one determines the memory address associated >> with the >>> + // lowest p_vaddr value for a PT_LOAD segment. This addre= ss is truncated to the nearest multiple of the maximum >> page size. >>> + // The corresponding p_vaddr value itself is also truncat= ed to the nearest multiple of the maximum page size. >>> + // >>> + // *** The base address is the difference between the tru= ncated memory address and the truncated p_vaddr value. >> *** >>> + // >>> + // Delta in this function is B. >>> + // >>> + // Calculation: B + A >>> + // >>> + if (RelaType =3D=3D SHT_RELA) { >>> + ASSERT (*Ptr =3D=3D 0); >>> + *Ptr =3D (UINT32) Delta + Rela->r_addend; >>> + } else { >>> + // >>> + // A is stored in the field of relocation for REL type. >>> + // >>> + *Ptr =3D (UINT32) Delta + *Ptr; >>> + } >>> + } else { >>> + // >>> + // non-Dynamic section doesn't contain entries of this type= . >>> + // >>> + DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", T= ype)); >>> + ASSERT (FALSE); >>> + } >>> + break; >>> + >>> + default: >>> + DEBUG ((DEBUG_INFO, "Unsupported relocation type %02X\n", Typ= e)); >>> + } >>> + } >> 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. OK, tyvm. > >>> + // >>> + // It's abnormal a DYN ELF doesn't contain a dynamic section. >>> + // >>> + ASSERT (DynShdr !=3D NULL); >>> + if (DynShdr =3D=3D NULL) { >>> + return EFI_UNSUPPORTED; >>> + } >>> + ASSERT (DynShdr->sh_type =3D=3D SHT_DYNAMIC); >>> + ASSERT (DynShdr->sh_entsize >=3D 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. I have started this discussion under another patch, maybe I should write= =20 a broader-scope mail to the list for comments. Basically using ASSERTs=20 for anything but *impossible* (*not* assuming the input data is=20 well-formed) situations significantly reduces the efficacy of dynamic=20 analysis. When doing fuzzing for example, you want to keep the ASSERTs=20 enabled to be made aware of any internal invariant violations. But if=20 ASSERTs happen on possible conditions, they will kill the fuzzing=20 process for no good reason. Turning them off will not analyse your=20 ASSERTs for possible code defects. Maybe fuzzing would be a good idea for this library? :) > > >>> + for ( Index =3D 0, Dyn =3D (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 >=3D sizeof (*Dyn). When you turn it into a runtime check as discussed above, yes. > > >>> + ASSERT (RelShdr->sh_type =3D=3D RelaType); >>> + ASSERT (RelShdr->sh_entsize =3D=3D RelaEntrySize); >> See above. >> > Agree. Will add if-checks. > > >>> + DEBUG ((DEBUG_INFO, "DYN ELF: Relocate using dynamic sections...\= n")); >>> + Status =3D 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. Same ASSERT point as above, "cannot" refers to both well-formed and=20 ill-formed images. > > >>> + return (Elf64_Phdr *)(ImageBase + Ehdr->e_phoff + Index * Ehdr->e_p= hentsize); >> Alignment checks? Bounds checks? >> > For the alignment checks, do you suggest that I should make sure the seg= ment should be placed > in the address that meets the alignment requirement? It could be implemented, PE code does it, but I meant pointer alignment=20 as discussed above somewhere. I don't think ELFs would likely request=20 more than page alignment, but abort + DEBUG message sounds like a good ide= a. > 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? Done above, sorry. > > >>> + >>> +/** >>> + 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 assum= ing > the ELF image is well-formatted. I get that idea, but the reality is that people will start using it for=20 external images once it is needed. :) Sorry for being pedantic. > > 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 !=3D NULL); >>> + >>> + Elf32Hdr =3D (Elf32_Ehdr *)ImageBase; >>> + >>> + // >>> + // Start with correct signature "\7fELF" >>> + // >>> + if ((Elf32Hdr->e_ident[EI_MAG0] !=3D ELFMAG0) || >>> + (Elf32Hdr->e_ident[EI_MAG1] !=3D ELFMAG1) || >>> + (Elf32Hdr->e_ident[EI_MAG1] !=3D ELFMAG1) || >>> + (Elf32Hdr->e_ident[EI_MAG2] !=3D ELFMAG2) >>> + ) { >>> + return FALSE; >>> + } >>> + >>> + // >>> + // Support little-endian only >>> + // >>> + if (Elf32Hdr->e_ident[EI_DATA] !=3D ELFDATA2LSB) { >>> + return FALSE; >>> + } >>> + >>> + // >>> + // Check 32/64-bit architecture >>> + // >>> + if (Elf32Hdr->e_ident[EI_CLASS] =3D=3D ELFCLASS64) { >>> + Elf64Hdr =3D (Elf64_Ehdr *)Elf32Hdr; >>> + Elf32Hdr =3D NULL; >>> + } else if (Elf32Hdr->e_ident[EI_CLASS] =3D=3D ELFCLASS32) { >>> + Elf64Hdr =3D 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 !=3D NULL) { >>> + // >>> + // Support intel architecture only for now >>> + // >>> + if (Elf64Hdr->e_machine !=3D EM_X86_64) { >>> + return FALSE; >>> + } >>> + > >>> + // Use last section as end of file >>> + Status =3D GetElfSectionPos (ElfCt, ElfCt->ShNum - 1, &Offset, &Siz= e); >> 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 =3D=3D ELFCLASS32) { >>> + Elf32Hdr =3D (Elf32_Ehdr *)ElfCt->FileBase; >>> + FileSize2 =3D Elf32Hdr->e_shoff + Elf32Hdr->e_shentsize * Elf32Hd= r->e_shnum; >>> + } else if (ElfCt->EiClass =3D=3D ELFCLASS64) { >>> + Elf64Hdr =3D (Elf64_Ehdr *)ElfCt->FileBase; >>> + FileSize2 =3D (UINTN)(Elf64Hdr->e_shoff + Elf64Hdr->e_shentsize *= Elf64Hdr->e_shnum); >>> + } >> Overflows? >> > Integer overflow? Yes, sorry. > Will add integer overflow check if this file size calculation logic is s= till needed. > > >>> + >>> + if (ElfCt =3D=3D 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 !=3D NULL)" above the if. > > >>> + ZeroMem (ElfCt, sizeof(ELF_IMAGE_CONTEXT)); >>> + >>> + if (ImageBase =3D=3D NULL) { >>> + return (ElfCt->ParseStatus =3D 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 ParseStatu= s > properly assigned before calling LoadElfImage(). But it just throws back the error without doing anything as far as I can= =20 see. For the new PE loader, there are "PeCoffInitializeContext" (more or= =20 less "ParseElfImage") and "PeCoffLoadImage" (more or less=20 "LoadElfImage"), and there is a precondition to not call latter when=20 former error'd. A minimal caller cal look like this: =C2=A0 Status =3D PeCoffInitializeContext (&Context, FileBuffer, FileSize= ); =C2=A0 if (RETURN_ERROR (Status)) { =C2=A0=C2=A0=C2=A0 return Status; =C2=A0 } =C2=A0 // [ ... hash image, allocate destination, and so on ... ] =C2=A0 PeCoffLoadImage (Context, Destination, DestinationSize); The load function is never invoked if the init function fails. This=20 gives an intuitive and easy-to-comprehend control flow. The old PE lib=20 also has a status member in the context, and it was one of the first=20 things I went away with. Callers should not read from the context, and=20 callees have clear contracts. > > 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 =3D (CHAR8 *)(ElfCt->FileBase + ElfCt->ShStrOff + Elf64Shd= r->sh_name); >> 0-termination checks, or return size? >> > I will validate the string section in ParseElfImage(). The validation lo= gic 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 s= tring section. > > >>> + >>> + ZeroMem (&Context, sizeof (Context)); >> This is done by the callee already. >> > Indeed. Will remove this. Rest looks good, thanks a lot! If you have some time, please consider checking the rest for similar=20 issues. I maybe could help with it if you would like that, but not right= =20 now, sorry. :) Best regards, Marvin > > >>> + Status =3D ParseElfImage (Elf, &Context); >>> + } while (EFI_ERROR (Status)); > >=20 > >