From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=MeNfltys; spf=pass (domain: linaro.org, ip: 209.85.166.196, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f196.google.com (mail-it1-f196.google.com [209.85.166.196]) by groups.io with SMTP; Wed, 17 Apr 2019 09:45:39 -0700 Received: by mail-it1-f196.google.com with SMTP id q14so5759836itk.0 for ; Wed, 17 Apr 2019 09:45:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2KxHNAovWRSLmSYNfEKRYHFxujpD1p2tv19UxC+Bxjk=; b=MeNfltyspQAgI48Dk3uNN2Et1TxCKB/NzLkTpij/ODCM8oSRo0/z4fqZaqwtVimfO6 5f52hYXGcUQqEWAJToSiQIgMFn01ERXSaE6Js1JNHujCSjhvjyuv0Pyi1+2Yxtni3Iqe RKE9xFY2f8Z8OhtAvhThVCmB/hIEQLmXOQUFWeAZqvI1nqHUE2jT02b04jF+k7sVvZ8t a7VsHEapDTDCrLJHEux3Xx4CnvC9tD8o4cbC1goqvQCygIh7vmJd0N/e+Ogz+LHQYNgF l19F79fqlmQ1khsfBKNGLXI6Xi/5jjFxfIO6GM6vfcw9BlbJtbE1iVaU3WZk+4USm6gy 7wGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2KxHNAovWRSLmSYNfEKRYHFxujpD1p2tv19UxC+Bxjk=; b=jL6Fv5emEbi0c7FBWFJgsY5gb1ujuAkLd/V00m/YGNPZ29tsxa7/HRYIhWO+EZ32YM nCMZ9Mq/8xxcaMMUHqz7xTf+HAP69Flbom0MGHhpvJkCCHbuqqBqb62QoICGd++kNRlZ VB8O76xcRkbfk5TxGbbqt7VP/PYsYZCiYDLFNF/CNCvdjTHZ7HSHWiufnswjgU+nxlrV OYIEW3BXkzUO68oyPtVXr0yWB9BTPREJEi9WpAoU08ENMVILlqvGsqicLY3mwCBZxR5H 8CkLJDOMGwmEq0kIiZljmMUoqlMzTk+S80aVySPeYqMmYkln0hr9DcENdMW2jMNWHlf3 +d4A== X-Gm-Message-State: APjAAAUh9Nrh3pTQ/1eHJ0/SCxMGVZuLaRRBi3q5CGfol6P3Rs/K53zW FH7qUipnpF0YQsE+Uz8/qFLMIFhBdBOpUo8GZVj9qw== X-Google-Smtp-Source: APXvYqyRx0RLg1Rls0liVfpGI/nvO44G5K/KbI6BcfRBTr2GVD9d6bvuiN/ttzBT2ngSI4MGXhhzv3ZoAYjQvccbk44= X-Received: by 2002:a24:59c1:: with SMTP id p184mr608374itb.158.1555519538246; Wed, 17 Apr 2019 09:45:38 -0700 (PDT) MIME-Version: 1.0 References: <20190417064047.4676-1-ard.biesheuvel@linaro.org> <735e04d0-08f2-ca4e-1074-9a6bd63406a2@redhat.com> In-Reply-To: <735e04d0-08f2-ca4e-1074-9a6bd63406a2@redhat.com> From: "Ard Biesheuvel" Date: Wed, 17 Apr 2019 09:45:26 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData To: Laszlo Ersek Cc: edk2-devel-groups-io , Jian J Wang , "Wu, Hao A" , "Ni, Ray" , Gary Ching-Pang Lin Content-Type: text/plain; charset="UTF-8" On Wed, 17 Apr 2019 at 06:09, Laszlo Ersek wrote: > > On 04/17/19 08:40, Ard Biesheuvel wrote: > > As reported by Gary, the recent LoadImage/StartImage changes to > > accommodate dispatching PE/COFF images built for foreign architectures > > may result in a crash when loading an IA32 option ROM into a X64 VM > > running OVMF: > > > > Loading driver at 0x0007E537000 EntryPoint=0x0007E53C06D 8086100e.efi > > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7F003B98 > > ProtectUefiImageCommon - 0x7F002BC0 > > - 0x000000007E537000 - 0x000000000009F900 > > Image type IA32 can't be started on X64 UEFI system. > > ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(698): Head->Signature == ((('p') | > > ('h' << 8)) | ((('d') | ('0' << 8)) << 16)) || Head->Signature > > == ((('p') | ('h' << 8)) | ((('d') | ('1' << 8)) << 16)) > > > > This turns out to be caused by the deferred image loading code in BDS, > > which doesn't check the result code of gBS->StartImage(), and ends up > > trying to free an uninitialized pointer. > > StartImage() can return an error status for one of two reasons: > - StartImage() itself fails, or > - StartImage() actually transfers control to the image, but then the > image exits with an error status. > > In the latter case, we have two further branches: > - the image produces the error status by returning from its entry point > function, or > - the image calls gBS->Exit(). > > In the last case, it is possible that the image exits with an error > *and* returns some ExitData that needs freeing. (In fact gBS->Exit() can > produce ExitData only if it returns an error.) > > Therefore, a failure status returned by gBS->StartImage() must not > prevent an attempt to free a non-NULL ExitData. > > My point being... the patch is correct, IMO, but the above paragraph of > the commit message has not been updated from v1, and it still suggests > (to me anyway) that things could be improved by adding a check on > "Status". That's not the case, IMO. > > So here's what I suggest: before pushing the patch, please simply remove > the fragment > > "doesn't check the result code of gBS->StartImage(), and" > Good point. > With that: > > Reviewed-by: Laszlo Ersek > Thanks Laszlo. I'll fix this before pushing the patch (assuming Hao is ok with this v2)