On Mon, 2012-11-12 at 21:55 +0100, Laszlo Ersek wrote:
> On 10/29/12 01:01, Jordan Justen wrote:
> > +BOOLEAN
> > +LooksLikeAPeCoffImage (
> > + IN VOID *Buffer,
> > + IN UINTN Size
> > + )
> > +{
> > + EFI_IMAGE_DOS_HEADER *DosHdr;
> > + EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
> > +
> > + ASSERT (Buffer != NULL);
>
> extraneous whitespace
>
> > +
> > + DosHdr = (EFI_IMAGE_DOS_HEADER *)Buffer;
>
> I'm not familiar with the headers you're checking here and I'll have to
> look them up, but I think we should make sure
>
> Size >= sizeof (EFI_IMAGE_DOS_HEADER)
>
> (also for the other headers) before converting the pointer.
>
> > + if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) {
> > + //
> > + // DOS image header is present, so read the PE header after the DOS
> > image header.
> > + //
> > + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINTN) Buffer + (UINTN)
> > ((DosHdr->e_lfanew) & 0x0ffff));
>
> (a) why not just 0xffff?
>
> (b) the expression relies on the following, thus I'd suggest checking it
> explicitly:
>
> Size >= ((DosHdr->e_lfanew & 0xffff) +
> sizeof (EFI_IMAGE_NT_HEADERS32))
>
> (The addition on the right side can't overflow on either supported
> platform.)
>
> > + } else {
> > + //
> > + // DOS image header is not present, so PE header is at the image base.
> > + //
> > + Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *) Buffer;
>
> Size >= sizeof (EFI_IMAGE_NT_HEADERS32)
>
> > + }
> > +
> > + //
> > + // Does the signature show a PE32 or TE image?
> > + //
> > + if ((Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) ||
> > + (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)) {
>
> After the additional checks suggested above, dereferencing Hdr.Pe32
> should be OK. However EFI_TE_IMAGE_HEADER could be bigger than
> EFI_IMAGE_NT_HEADERS32 (it seems to be the case in fact), for which case
> we have to redo the same checks.
>
> > + return TRUE;
> > + } else {
> > + return FALSE;
> > + }
> > +}
>
> I suggest rewriting the function like this:
>
> BOOLEAN
> LooksLikeAPeCoffImage (
> IN VOID *Buffer,
> IN UINTN Size
> )
> {
> EFI_IMAGE_DOS_HEADER *DosHdr;
> UINTN Offset;
> EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr;
>
> ASSERT (Buffer != NULL);
>
> if (Size < sizeof (EFI_IMAGE_DOS_HEADER)) {
> return FALSE;
> }
> DosHdr = (EFI_IMAGE_DOS_HEADER *) Buffer;
>
> //
> // If DOS image header is present, search for the PE header after the DOS
> // image.
> //
> Offset = (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) ?
> (DosHdr->e_lfanew) & 0xffff :
> 0;
>
> //
> // The following check is for a sufficient (not necessary) condition, but
> in
> // practice it should never refuse a valid image.
> // The addition on the RHS never overflows.
> //
> if (Size < Offset + sizeof (EFI_IMAGE_OPTIONAL_HEADER_UNION)) {
> return FALSE;
> }
> Hdr.Union = (EFI_IMAGE_OPTIONAL_HEADER_UNION *) ((UINT8 *) Buffer +
> Offset);
>
> //
> // Does the signature show a PE32 or TE image?
> //
> if ((Hdr.Te->Signature == EFI_TE_IMAGE_HEADER_SIGNATURE) ||
> (Hdr.Pe32->Signature == EFI_IMAGE_NT_SIGNATURE)) {
> return TRUE;
> } else {
> return FALSE;
> }
> }
You might notice some resemblance between LooksLikeAPeCoffImage and
MdePkg/Library/BasePeCoffGetEntryPointLib/PeCoffGetEntryPoint.c
=> PeCoffLoaderGetEntryPoint. :)
Perhaps similar changes would be valuable for that code? (Uh, I'll
leave this part up to you... :)
I almost used PeCoffGetEntryPointLib, even though I don't need
the actual entry point address. I still like the idea, so maybe
I'll do that in v2.
-Jordan
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
