Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

["Michael Kubacki" <mikuback@linux.microsoft.com>]

On 11/15/2023 5:10 AM, Laszlo Ersek wrote:
> On 11/14/23 20:37, Michael Kubacki wrote:
>> On 11/14/2023 11:21 AM, Michael D Kinney wrote:
>>> Hi Ranbir,
>>>
>>> First I want to recognize your efforts to collect Coverity issues and
>>> propose changes to address
>>>
>>> them.
>>>
>>> I still disagree with adding CpuDealLoop() for any static analysis
>>> issues.
>>>
>>> There have been previous discussions about adding a PANIC() or FATAL()
>>> macro that would
>>>
>>> perform platform specific actions if a condition is detected where the
>>> boot of the platform
>>>
>>> can not continue.  A platform get to make the choice to log or reboot
>>> or hang, etc.  Not the
>>>
>>> code that detected the condition.
>>>
>>> https://edk2.groups.io/g/devel/message/86597
>>> <https://edk2.groups.io/g/devel/message/86597>
>>>
>> After going through hundreds of edk2 static analysis findings, we found
>> a small number of cases where an interface such as PanicLib was useful
>> and recently added an implementation
>> https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h.
>>
>> For example, the return value from calls to MpInitLibWhoAmI() in
>> exception related code often goes unchecked and it's been used there.
>> Being able to separate the library instance implementation linked to a
>> given module from a more broad library class like DebugLib for these
>> cases has also been helpful.
> 
> Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally
> forgotten about that; my apologies.
> 
> ... I initially thought that a plain "CONST CHAR8  *Description" was not
> too flexible, but on second thought, it should be exactly right. Reason
> being, it's very easy to print. Format specifiers and variable arguments
> (PrintLib style) may be too complex to implement safely within
> PanicReport(). Arguably, no PanicReport() implementation should be
> obligated (by the interface) to depend on, or to reimplement, PrintLib.
> If the calling context permits, the caller can just use PrintLib to
> format the message to a local buffer (on the stack), and pass that to
> PanicReport.
> 
> So this looks very useful to me; can you upstream it?
> 
Thanks for taking a look. We definitely want to align with edk2 on a 
consistent way to handle these scenarios. We'll send a patch for further 
discussion and review.

We've waited to do so to allow the original author (Ken Lautner) to 
return from his time out of office to send that patch.

- Michael

> Laszlo
> 
> 
>>
>>> Unfortunately, in order to fix some of these static analysis issues
>>> correctly, we are going
>>>
>>> to have to identify the use of ASSERT() that really is a fatal
>>> condition that can not continue
>>>
>>> and introduce an implementation approach that provides a platform
>>> handler and
>>>
>>> satisfies the static analysis tools.
>>>
>>> We also have to evaluate if a return error status with a DEBUG_ERROR
>>> message would be a better
>>>
>>> choice than an ASSERT() that can be filtered out by build options.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
>>> *Ranbir Singh
>>> *Sent:* Tuesday, November 14, 2023 7:08 AM
>>> *To:* Laszlo Ersek <lersek@redhat.com>
>>> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh
>>> Sangolli <veeresh.sangolli@dellteam.com>
>>> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
>>> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>>>
>>> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com
>>> <mailto:lersek@redhat.com>> wrote:
>>>
>>>      On 11/10/23 07:11, Ranbir Singh wrote:
>>>       > EFI_NOT_READY was listed as one of the error return values in the
>>>       > function header of StartPciDevices(). So, I considered it here.
>>>       >
>>>       > Maybe we can go by a dual approach, including CpuDeadLoop() in
>>>       > StartPciDevices() as well as add return value check at the call
>>>      site in
>>>       > PciBusDriverBindingStart().
>>>
>>>      I don't think this makes much sense, given that execution is not
>>>      supposed to continue past CpuDeadLoop(), so the new error check would
>>>      not be reached.
>>>
>>>      I think two options are realistic:
>>>
>>>      - replace the assert() with a CpuDeadLoop() -- this is my preference
>>>
>>>      - keep the assert, add the "if", and in the caller, handle the
>>>      EFI_NOT_READY error. This is workable too. (Keeping the assert is
>>> goog
>>>      because it shows that we don't expect the "if" to fire.)
>>>
>>>      Anyway, now that we have no way to verify the change against
>>> Coverity, I
>>>      don't know if this patch is worth the churn. :( As I said, this
>>> ASSERT()
>>>      is one of those few justified ones in edk2 core that can indeed never
>>>      fail, IMO.
>>>
>>>      Laszlo
>>>
>>> See, Does the following change look acceptable to you ?
>>>
>>>       ASSERT (RootBridge != NULL);
>>> +  if (RootBridge == NULL) {
>>>
>>> +    CpuDeadLoop ();
>>> +    return EFI_NOT_READY;
>>> +  }
>>>
>>> +
>>>
>>> which retains the existing assert, adds the NULL pointer check and
>>> includes CpuDeadLoop () within the if block. As a result of
>>> CpuDeadLoop (), the return statement afterwards will never reach
>>> execution (=> no need to handle this return value at the call sites),
>>> but will satisfy static analysis tools as the "RootBridge" dereference
>>> scenario doesn't arise thereafter.
>>>
>>>
>>>       >
>>>       > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
>>>      <mailto:lersek@redhat.com>
>>>       > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
>>>       >
>>>       >     On 11/7/23 07:19, Ranbir Singh wrote:
>>>       >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com
>>>      <mailto:Ranbir.Singh3@Dell.com>>
>>>       >     >
>>>       >     > The function StartPciDevices has a check
>>>       >     >
>>>       >     >     ASSERT (RootBridge != NULL);
>>>       >     >
>>>       >     > but this comes into play only in DEBUG mode. In Release
>>>      mode, there
>>>       >     > is no handling if the RootBridge value is NULL and the code
>>>      proceeds
>>>       >     > to unconditionally dereference "RootBridge" which will lead
>>>      to CRASH.
>>>       >     >
>>>       >     > Hence, for safety add NULL pointer checks always and return
>>>       >     > EFI_NOT_READY if RootBridge value is NULL which is one of
>>>      the return
>>>       >     > values as mentioned in the function description header.
>>>       >     >
>>>       >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>>      <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>>>       >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>>      <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>>
>>>       >     >
>>>       >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>
>>>      <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>
>>>       >     > Co-authored-by: Veeresh Sangolli
>>>      <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com>
>>>       >     <mailto:veeresh.sangolli@dellteam.com
>>>      <mailto:veeresh.sangolli@dellteam.com>>>
>>>       >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com
>>>      <mailto:Ranbir.Singh3@Dell.com>>
>>>       >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>>>      <mailto:rsingh@ventanamicro.com>
>>>       >     <mailto:rsingh@ventanamicro.com
>>>      <mailto:rsingh@ventanamicro.com>>>
>>>       >     > ---
>>>       >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>>>       >     >  1 file changed, 4 insertions(+), 1 deletion(-)
>>>       >     >
>>>       >     > diff --git
>>> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > index 581e9075ad41..3de80d98370e 100644
>>>       >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > @@ -772,7 +772,10 @@ StartPciDevices (
>>>       >     >    LIST_ENTRY     *CurrentLink;
>>>       >     >
>>>       >     >    RootBridge = GetRootBridgeByHandle (Controller);
>>>       >     > -  ASSERT (RootBridge != NULL);
>>>       >     > +  if (RootBridge == NULL) {
>>>       >     > +    return EFI_NOT_READY;
>>>       >     > +  }
>>>       >     > +
>>>       >     >    ThisHostBridge =
>>> RootBridge->PciRootBridgeIo->ParentHandle;
>>>       >     >
>>>       >     >    CurrentLink = mPciDevicePool.ForwardLink;
>>>       >
>>>       >     I don't think this is a good fix.
>>>       >
>>>       >     There is one call site, namely in PciBusDriverBindingStart().
>>>      That call
>>>       >     site does not check the return value. (Of course /s)
>>>       >
>>>       >     I think that this ASSERT() can indeed never fail. Therefore I
>>>      suggest
>>>       >     CpuDeadLoop() instead.
>>>       >
>>>       >     If you insist that CpuDeadLoop() is "too risky" here, then
>>>      the patch is
>>>       >     acceptable, but then the StartPciDevices() call site in
>>>       >     PciBusDriverBindingStart() must check the error properly: we
>>>      must not
>>>       >     install "gEfiPciEnumerationCompleteProtocolGuid", and the
>>>      function must
>>>       >     propagate the error outwards.
>>>       >
>>>       >     Laszlo
>>>       >
>>>
>>>
>>
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111482): https://edk2.groups.io/g/devel/message/111482
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-