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=Bc/qxfkV; 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 18:04:50 -0700 Received: by mail-it1-f196.google.com with SMTP id q14so819307itk.0 for ; Wed, 17 Apr 2019 18:04:50 -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=JOn1Gd/pqEiJ1pnymyIrTXZvBRMWGA5VWjaVhbWeTEU=; b=Bc/qxfkVXXGka267XUPyX1n7RVu4JHbiZlnYtJE4cyFyq39D/hKwp4Vr8UJuU/7Jkf 3i4YGaQ1vL8j/Tbii/2nkGabU14CgrUz24bSPQfY8gp7d5uaMIc0A1+/9eMKg5YvUQMg XtbfQthQyTTFM2DwmcPbX1SRBW2v/mN7lks8p/aOFPZXtJz6GY+Helm5Xoc1Q9I2p+ox QReKUBeZiG0Q58RYbrj8Ra9pfKpYsv9K7Ci7G4vlieyCJJSKwj9UICxvUOnK6mfF50et /89DyhdB9o++lgCSVwD7+BDQ+3BEuVmrBtDvYyhTMmOjrtxHW9DHzgRQrWLf89RH9StH /6bw== 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=JOn1Gd/pqEiJ1pnymyIrTXZvBRMWGA5VWjaVhbWeTEU=; b=UntKstAwyJN/QY87Wjfq8eAEYF9L0TFMkK/7/okNXFAufNDu0gJXHTn17kaX2FEGIA IitsIyVFmpiRxKkRjzAPG5/ljzItbDOf5Q9iNq3nhxTSwizY2gGyOFsA/L7fVzqlrQRz xxLtfw/JYNi4ju/cTo4mUSAqLyWG6ny9Eq7+xZfEQ5ueN+Jor3PoRWwvCP0YNNh/843O 1+yKJ8ibYCXXdH2DzOktkCT2gsV9aXoRHm/fb+JFzVOTRgNCtOA/GAscN3LXdJJhqN09 xi9gWF/zZN0TI5FTnqcNotRfndT/C7XksaaducE15zYRMCtatbqMgfYaBs9O5BwoNTkx wgXg== X-Gm-Message-State: APjAAAWXk9AzQOmu0LLTjAu9dbdX85/ZCfa0vSXxn5BkB/jqtmK1920N AJQicyPVELnqU8RHKf19iIXnJ43ELtNafQ8Ngz4ygg== X-Google-Smtp-Source: APXvYqzAtRxzaoBBFIBmMEtcZ47NLoUhzxIkBt+hAsaIFzA13FCK/RtPLI5KvL1Oc7AmeGBamNgQsY0TnMNWfBpEjZ0= X-Received: by 2002:a05:6638:60e:: with SMTP id g14mr35763118jar.62.1555549489466; Wed, 17 Apr 2019 18:04:49 -0700 (PDT) MIME-Version: 1.0 References: <20190417064047.4676-1-ard.biesheuvel@linaro.org> <735e04d0-08f2-ca4e-1074-9a6bd63406a2@redhat.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 17 Apr 2019 18:04:37 -0700 Message-ID: Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData To: "Wu, Hao A" Cc: Laszlo Ersek , edk2-devel-groups-io , "Wang, Jian J" , "Ni, Ray" , Gary Ching-Pang Lin Content-Type: text/plain; charset="UTF-8" On Wed, 17 Apr 2019 at 18:00, Wu, Hao A wrote: > > > -----Original Message----- > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] > > Sent: Thursday, April 18, 2019 12:45 AM > > To: Laszlo Ersek > > Cc: edk2-devel-groups-io; Wang, Jian J; Wu, Hao A; Ni, Ray; Gary Ching-Pang > > Lin > > Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix > > crash on uninitialized ExitData > > > > 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" > > > > > Agree with Laszlo. > With the refining the message, > Reviewed-by: Hao Wu > Thanks all Pushed as cfb29d2bda57..d43056888790