From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-x22d.google.com (mail-it0-x22d.google.com [IPv6:2607:f8b0:4001:c0b::22d]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 9EDA620958961 for ; Wed, 5 Jul 2017 09:44:19 -0700 (PDT) Received: by mail-it0-x22d.google.com with SMTP id k192so90038520ith.1 for ; Wed, 05 Jul 2017 09:45:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8PeNsinLaqtg5GQ29uvLRmYbRytRUSo5m+zZtZSHf5s=; b=JVVxyBnWs3IpcRp7orGJPi6RuibEjr4edisSsWjvrKyTAC0NhZ3T5QuBOdoNpjZxdR +I8F7+6Xb1TlSZVCtDMY7qbTxyDxoeYBqF5AR6xjCWZ6YMgJ2ICVnvMK5bbA7Cjy6Jl/ +NHS27Y+Qja228RhGC5fumzneZTwYkAqc8rG4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8PeNsinLaqtg5GQ29uvLRmYbRytRUSo5m+zZtZSHf5s=; b=CY/wFdaVx5/m+t5i2pLIgSrQPfoe5IRMd7uUWYf7wzWdMGJVBI3cwhSjwohcLqyGer EbqOZbZYEQiaGxK7QtpcWaXR8U3Yq9Zqgk9iZSE7A6JOgiag4SC8MrADnBjsl2nRqsPE Gk7nPHPXQwT9Z0Ty+pLnKzKAJj6qNrcTX+c8bLcg7Ux6tguB7Wdj/RqjxqIyhQK68WYj 9mfP4KibieHU36BGVllZiQy63gZmwouAMiV40GOLwn6PvVwjsjwJDHL3SrNVEzIChXBQ ThkbH9FOC21hOA0viK76ymKeGJWEyWIxyXVzPyryI/SEkcpw5ou++Pkmr7nZwubnzfGU NGRg== X-Gm-Message-State: AKS2vOwxENp6UbhPKCqjO8x00myyPcxVaog+6wDCV0269Cv2WiAsx3SE n9YtuPi84RUW/SbLQWz1PSyJhYDfu+1K X-Received: by 10.107.59.84 with SMTP id i81mr41357321ioa.72.1499273158514; Wed, 05 Jul 2017 09:45:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.134.134 with HTTP; Wed, 5 Jul 2017 09:45:57 -0700 (PDT) In-Reply-To: <20170705164218.25814-1-lersek@redhat.com> References: <20170705164218.25814-1-lersek@redhat.com> From: Ard Biesheuvel Date: Wed, 5 Jul 2017 17:45:57 +0100 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Gerd Hoffmann , Leif Lindholm , Liming Gao , Yonghong Zhu Subject: Re: [PATCH] BaseTools/GenFw: work around GNU Binutils bug wrt. DebugDirectoryEntrySize X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Jul 2017 16:44:19 -0000 Content-Type: text/plain; charset="UTF-8" On 5 July 2017 at 17:42, Laszlo Ersek wrote: > GNU Binutils produce a PE debug directory with one This sentence already confuses me. This crash is reproducible on ARM, but the ARM toolchains are strictly ELF based, and all PE/COFF data structures are created by GenFw itself, never by binutils. So I don't see how this could be a binutils bug. > EFI_IMAGE_DEBUG_DIRECTORY_ENTRY: > - the Type field of the entry is EFI_IMAGE_DEBUG_TYPE_CODEVIEW, > - the FileOffset field of the entry points right past the entry itself, > - the data structure placed at FileOffset is a CV_INFO_PDB20 structure, > with an "NB10" signature. > > This is all correct, except GNU Binutils include the pointed-to > CV_INFO_PDB20 structure in the size of the debug directory (that is, > Optional64Hdr->DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_DEBUG].Size). > That's a bug. > > The malformed debug directory size causes the loop in GenFw's > ZeroDebugData() function to process the CV_INFO_PDB20 structure as a set > of EFI_IMAGE_DEBUG_DIRECTORY_ENTRY elements, which crashes GenFw. > > This problem was exposed by commit e4129b0e5897 ("BaseTools: Update GenFw > to clear unused debug entry generated by VS tool chain", 2017-06-19). > > Work around the Binutils issue by noticing when an > EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset points back into the debug > directory. (This can never happen with a well-formed PE file.) In this > case, truncate DebugDirectoryEntrySize such that the debug directory will > end right before the debug structure pointed-to by > EFI_IMAGE_DEBUG_DIRECTORY_ENTRY.FileOffset. > > Tested with OVMF: > - gcc-4.8.5-14.el7.x86_64 > - binutils-2.25.1-27.base.el7.x86_64 > > and with ArmVirtPkg: > - gcc-aarch64-linux-gnu-6.1.1-2.el7.x86_64 > - binutils-aarch64-linux-gnu-2.27-3.el7.x86_64 > > Cc: Ard Biesheuvel > Cc: Gerd Hoffmann > Cc: Leif Lindholm > Cc: Liming Gao > Cc: Yonghong Zhu > Reported-by: Gerd Hoffmann > Reported-by: Leif Lindholm > Ref: http://mid.mail-archive.com/a1de67a8-57c2-908e-dd4d-9726d60fb388@redhat.com > Ref: http://mid.mail-archive.com/20170705134136.GB26676@bivouac.eciton.net > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: binutils_debugdirsize_workaround > > BaseTools/Source/C/GenFw/GenFw.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/BaseTools/Source/C/GenFw/GenFw.c b/BaseTools/Source/C/GenFw/GenFw.c > index 6569460f34f7..a79f485ee681 100644 > --- a/BaseTools/Source/C/GenFw/GenFw.c > +++ b/BaseTools/Source/C/GenFw/GenFw.c > @@ -2771,6 +2771,7 @@ Returns: > UINT32 Index; > UINT32 DebugDirectoryEntryRva; > UINT32 DebugDirectoryEntrySize; > + UINT32 TruncatedDebugDirectorySize; > UINT32 DebugDirectoryEntryFileOffset; > UINT32 ExportDirectoryEntryRva; > UINT32 ExportDirectoryEntryFileOffset; > @@ -2893,6 +2894,25 @@ Returns: > DebugEntry = (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY *) (FileBuffer + DebugDirectoryEntryFileOffset); > Index = 0; > for (Index=0; Index < DebugDirectoryEntrySize / sizeof (EFI_IMAGE_DEBUG_DIRECTORY_ENTRY); Index ++, DebugEntry ++) { > + // > + // Work around GNU Binutils bug: if the debug information pointed-to by > + // DebugEntry was incorrectly included in DebugDirectoryEntrySize, then > + // the debug directory doesn't actually extend past the pointed-to debug > + // information. Truncate DebugDirectoryEntrySize accordingly. > + // > + if (DebugEntry->FileOffset >= DebugDirectoryEntryFileOffset && > + DebugEntry->FileOffset < (DebugDirectoryEntryFileOffset + > + DebugDirectoryEntrySize)) { > + TruncatedDebugDirectorySize = (DebugEntry->FileOffset - > + DebugDirectoryEntryFileOffset); > + VerboseMsg ( > + "truncating debug directory size from %u to %u", > + DebugDirectoryEntrySize, > + TruncatedDebugDirectorySize > + ); > + DebugDirectoryEntrySize = TruncatedDebugDirectorySize; > + } > + > DebugEntry->TimeDateStamp = 0; > if (ZeroDebugFlag || DebugEntry->Type != EFI_IMAGE_DEBUG_TYPE_CODEVIEW) { > memset (FileBuffer + DebugEntry->FileOffset, 0, DebugEntry->SizeOfData); > -- > 2.13.1.3.g8be5a757fa67 >