public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael D Kinney" <michael.d.kinney@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
	"rsingh@ventanamicro.com" <rsingh@ventanamicro.com>,
	Laszlo Ersek <lersek@redhat.com>,
	"Andrew Fish (afish@apple.com)" <afish@apple.com>,
	"quic_llindhol@quicinc.com" <quic_llindhol@quicinc.com>,
	Michael Kubacki <mikuback@linux.microsoft.com>
Cc: "Ni, Ray" <ray.ni@intel.com>,
	Veeresh Sangolli <veeresh.sangolli@dellteam.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
Date: Tue, 14 Nov 2023 16:21:30 +0000	[thread overview]
Message-ID: <CO1PR11MB4929DF722DED984ADCEB5170D2B2A@CO1PR11MB4929.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CAA9DWXCGo6tDi2qLU2fJnteWJ6NxKerN-FFVaQAWX43Az6ngpA@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 6674 bytes --]

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

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>
>     >
>     > 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 (#111207): https://edk2.groups.io/g/devel/message/111207
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 13770 bytes --]

  reply	other threads:[~2023-11-14 16:21 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-07  6:19 [edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue Ranbir Singh
2023-11-07 16:21   ` Laszlo Ersek
2023-11-10  6:14     ` Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues Ranbir Singh
2023-11-07  8:15   ` Ni, Ray
2023-11-07 16:23   ` Laszlo Ersek
2023-11-07 17:59     ` Michael D Kinney
2023-11-08  3:51       ` Ranbir Singh
2023-11-08  4:05         ` Michael D Kinney
2023-11-08  4:29           ` Ranbir Singh
2023-11-13 11:24             ` Laszlo Ersek
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON " Ranbir Singh
2023-11-07 16:41   ` Laszlo Ersek
2023-11-10  5:59     ` Ranbir Singh
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue Ranbir Singh
2023-11-07 16:48   ` Laszlo Ersek
2023-11-10  6:11     ` Ranbir Singh
2023-11-13 11:28       ` Laszlo Ersek
2023-11-14 15:08         ` Ranbir Singh
2023-11-14 16:21           ` Michael D Kinney [this message]
2023-11-14 16:53             ` Ranbir Singh
2023-11-15 10:02               ` Laszlo Ersek
2023-11-14 19:37             ` Michael Kubacki
2023-11-15 10:10               ` Laszlo Ersek
2023-11-20 14:05                 ` Michael Kubacki
2023-11-15  9:50             ` Laszlo Ersek
2023-11-20  3:57               ` Ranbir Singh
2023-11-21 17:07                 ` Laszlo Ersek
2023-11-07  6:19 ` [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues Ranbir Singh
2023-11-07 17:20   ` Laszlo Ersek
2023-11-10  6:31     ` Ranbir Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CO1PR11MB4929DF722DED984ADCEB5170D2B2A@CO1PR11MB4929.namprd11.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox