* [edk2-devel] [PATCH v3 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity @ 2023-11-10 5:22 Ranbir Singh 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues Ranbir Singh 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK " Ranbir Singh 0 siblings, 2 replies; 5+ messages in thread From: Ranbir Singh @ 2023-11-10 5:22 UTC (permalink / raw) To: devel, rsingh v2 -> v3: - Updated FORWARD_NULL issue - Included DEBUG message and CpuDeadLoop within if blocks - Updated MISSING_BREAK issues resolution - Merged fallthrough case with default with conditional messaging Ranbir Singh (2): MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 29 +++++++++++++++++ MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++--------- 2 files changed, 48 insertions(+), 14 deletions(-) -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111011): https://edk2.groups.io/g/devel/message/111011 Mute This Topic: https://groups.io/mt/102502054/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 5+ messages in thread
* [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues 2023-11-10 5:22 [edk2-devel] [PATCH v3 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity Ranbir Singh @ 2023-11-10 5:22 ` Ranbir Singh 2023-11-13 16:57 ` Laszlo Ersek 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK " Ranbir Singh 1 sibling, 1 reply; 5+ messages in thread From: Ranbir Singh @ 2023-11-10 5:22 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli From: Ranbir Singh <Ranbir.Singh3@Dell.com> The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr and UsbHcFreeMem do have ASSERT ((Block != NULL)); statements after for loop, but these are applicable only in DEBUG mode. In RELEASE mode, if for whatever reasons there is no match inside for loop and the loop exits because of Block != NULL; condition, then there is no "Block" NULL pointer check afterwards and the code proceeds to do dereferencing "Block" which will lead to CRASH. Hence, for safety add NULL pointer checks always. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221 Cc: Ray Ni <ray.ni@intel.com> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> --- MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 29 ++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c index b54187ec228e..597cbe4646e8 100644 --- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c @@ -267,6 +267,16 @@ UsbHcGetPciAddrForHostAddr ( } ASSERT ((Block != NULL)); + + if (Block == NULL) { + // + // Should never be here + // + DEBUG ((DEBUG_ERROR, "UsbHcGetPciAddrForHostAddr: Invalid host memory pointer passed\n")); + CpuDeadLoop (); + return 0; + } + // // calculate the pci memory address for host memory address. // @@ -322,6 +332,16 @@ UsbHcGetHostAddrForPciAddr ( } ASSERT ((Block != NULL)); + + if (Block == NULL) { + // + // Should never be here + // + DEBUG ((DEBUG_ERROR, "UsbHcGetHostAddrForPciAddr: Invalid pci memory pointer passed\n")); + CpuDeadLoop (); + return 0; + } + // // calculate the pci memory address for host memory address. // @@ -603,6 +623,15 @@ UsbHcFreeMem ( // ASSERT (Block != NULL); + if (Block == NULL) { + // + // Should never be here + // + DEBUG ((DEBUG_ERROR, "UsbHcFreeMem: Invalid memory pointer passed\n")); + CpuDeadLoop (); + return; + } + // // Release the current memory block if it is empty and not the head // -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111012): https://edk2.groups.io/g/devel/message/111012 Mute This Topic: https://groups.io/mt/102502055/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues Ranbir Singh @ 2023-11-13 16:57 ` Laszlo Ersek 0 siblings, 0 replies; 5+ messages in thread From: Laszlo Ersek @ 2023-11-13 16:57 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni, Veeresh Sangolli On 11/10/23 06:22, Ranbir Singh wrote: > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > The functions UsbHcGetHostAddrForPciAddr, UsbHcGetPciAddrForHostAddr > and UsbHcFreeMem do have > > ASSERT ((Block != NULL)); > > statements after for loop, but these are applicable only in DEBUG mode. > In RELEASE mode, if for whatever reasons there is no match inside for > loop and the loop exits because of Block != NULL; condition, then there > is no "Block" NULL pointer check afterwards and the code proceeds to do > dereferencing "Block" which will lead to CRASH. > > Hence, for safety add NULL pointer checks always. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221 > > Cc: Ray Ni <ray.ni@intel.com> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> > --- > MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c | 29 ++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c > index b54187ec228e..597cbe4646e8 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/UsbHcMem.c > @@ -267,6 +267,16 @@ UsbHcGetPciAddrForHostAddr ( > } > > ASSERT ((Block != NULL)); > + > + if (Block == NULL) { > + // > + // Should never be here > + // > + DEBUG ((DEBUG_ERROR, "UsbHcGetPciAddrForHostAddr: Invalid host memory pointer passed\n")); > + CpuDeadLoop (); > + return 0; > + } > + > // > // calculate the pci memory address for host memory address. > // > @@ -322,6 +332,16 @@ UsbHcGetHostAddrForPciAddr ( > } > > ASSERT ((Block != NULL)); > + > + if (Block == NULL) { > + // > + // Should never be here > + // > + DEBUG ((DEBUG_ERROR, "UsbHcGetHostAddrForPciAddr: Invalid pci memory pointer passed\n")); > + CpuDeadLoop (); > + return 0; > + } > + > // > // calculate the pci memory address for host memory address. > // > @@ -603,6 +623,15 @@ UsbHcFreeMem ( > // > ASSERT (Block != NULL); > > + if (Block == NULL) { > + // > + // Should never be here > + // > + DEBUG ((DEBUG_ERROR, "UsbHcFreeMem: Invalid memory pointer passed\n")); > + CpuDeadLoop (); > + return; > + } > + > // > // Release the current memory block if it is empty and not the head > // Reviewed-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111166): https://edk2.groups.io/g/devel/message/111166 Mute This Topic: https://groups.io/mt/102502055/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 5+ messages in thread
* [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues 2023-11-10 5:22 [edk2-devel] [PATCH v3 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity Ranbir Singh 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues Ranbir Singh @ 2023-11-10 5:22 ` Ranbir Singh 2023-11-13 17:00 ` Laszlo Ersek 1 sibling, 1 reply; 5+ messages in thread From: Ranbir Singh @ 2023-11-10 5:22 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni From: Ranbir Singh <Ranbir.Singh3@Dell.com> The functions XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has a switch-case code in which the case USB_ENDPOINT_CONTROL: falls through to default: While this may be intentional, it may not be evident to any general code reader/developer or static analyis tool why there is no break in between. Merge the USB_ENDPOINT_CONTROL and default using conditional debug print. REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221 Cc: Ray Ni <ray.ni@intel.com> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> --- MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++--------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c index 05528a478baf..00b3a13a95bb 100644 --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c @@ -2825,6 +2825,7 @@ XhcInitializeEndpointContext ( UINTN NumEp; UINTN EpIndex; UINT8 EpAddr; + UINT8 EpType; UINT8 Direction; UINT8 Dci; UINT8 MaxDci; @@ -2871,7 +2872,8 @@ XhcInitializeEndpointContext ( InputContext->EP[Dci-1].MaxBurstSize = 0x0; } - switch (EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK) { + EpType = EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK; + switch (EpType) { case USB_ENDPOINT_BULK: if (Direction == EfiUsbDataIn) { InputContext->EP[Dci-1].CErr = 3; @@ -2974,13 +2976,13 @@ XhcInitializeEndpointContext ( break; - case USB_ENDPOINT_CONTROL: - // - // Do not support control transfer now. - // - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n")); default: - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unknown EP found, Transfer ring is not allocated.\n")); + DEBUG (( + DEBUG_INFO, + "%a: %a found, Transfer ring is not allocated.\n", + __func__, + (EpType == USB_ENDPOINT_CONTROL ? "Unsupported Control EP" : "Unknown EP") + )); EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length); continue; } @@ -3028,6 +3030,7 @@ XhcInitializeEndpointContext64 ( UINTN NumEp; UINTN EpIndex; UINT8 EpAddr; + UINT8 EpType; UINT8 Direction; UINT8 Dci; UINT8 MaxDci; @@ -3074,7 +3077,8 @@ XhcInitializeEndpointContext64 ( InputContext->EP[Dci-1].MaxBurstSize = 0x0; } - switch (EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK) { + EpType = EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK; + switch (EpType) { case USB_ENDPOINT_BULK: if (Direction == EfiUsbDataIn) { InputContext->EP[Dci-1].CErr = 3; @@ -3177,13 +3181,14 @@ XhcInitializeEndpointContext64 ( break; - case USB_ENDPOINT_CONTROL: - // - // Do not support control transfer now. - // - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unsupport Control EP found, Transfer ring is not allocated.\n")); default: - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unknown EP found, Transfer ring is not allocated.\n")); + DEBUG (( + DEBUG_INFO, + "%a: %a found, Transfer ring is not allocated.\n", + __func__, + ((EpType == USB_ENDPOINT_CONTROL) ? "Unsupported Control EP" : "Unknown EP") + )); + EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length); continue; } -- 2.34.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111013): https://edk2.groups.io/g/devel/message/111013 Mute This Topic: https://groups.io/mt/102502056/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK Coverity issues 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK " Ranbir Singh @ 2023-11-13 17:00 ` Laszlo Ersek 0 siblings, 0 replies; 5+ messages in thread From: Laszlo Ersek @ 2023-11-13 17:00 UTC (permalink / raw) To: devel, rsingh; +Cc: Ray Ni On 11/10/23 06:22, Ranbir Singh wrote: > From: Ranbir Singh <Ranbir.Singh3@Dell.com> > > The functions > XhcInitializeEndpointContext and XhcInitializeEndpointContext64 has > a switch-case code in which the case USB_ENDPOINT_CONTROL: falls > through to default: > > While this may be intentional, it may not be evident to any general code > reader/developer or static analyis tool why there is no break in between. > > Merge the USB_ENDPOINT_CONTROL and default using conditional debug print. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4221 > > Cc: Ray Ni <ray.ni@intel.com> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com> > --- > MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c | 33 +++++++++++--------- > 1 file changed, 19 insertions(+), 14 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > index 05528a478baf..00b3a13a95bb 100644 > --- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > +++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c > @@ -2825,6 +2825,7 @@ XhcInitializeEndpointContext ( > UINTN NumEp; > UINTN EpIndex; > UINT8 EpAddr; > + UINT8 EpType; > UINT8 Direction; > UINT8 Dci; > UINT8 MaxDci; > @@ -2871,7 +2872,8 @@ XhcInitializeEndpointContext ( > InputContext->EP[Dci-1].MaxBurstSize = 0x0; > } > > - switch (EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK) { > + EpType = EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK; > + switch (EpType) { > case USB_ENDPOINT_BULK: > if (Direction == EfiUsbDataIn) { > InputContext->EP[Dci-1].CErr = 3; > @@ -2974,13 +2976,13 @@ XhcInitializeEndpointContext ( > > break; > > - case USB_ENDPOINT_CONTROL: > - // > - // Do not support control transfer now. > - // > - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unsupport Control EP found, Transfer ring is not allocated.\n")); > default: > - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext: Unknown EP found, Transfer ring is not allocated.\n")); > + DEBUG (( > + DEBUG_INFO, > + "%a: %a found, Transfer ring is not allocated.\n", > + __func__, > + (EpType == USB_ENDPOINT_CONTROL ? "Unsupported Control EP" : "Unknown EP") > + )); > EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length); > continue; > } > @@ -3028,6 +3030,7 @@ XhcInitializeEndpointContext64 ( > UINTN NumEp; > UINTN EpIndex; > UINT8 EpAddr; > + UINT8 EpType; > UINT8 Direction; > UINT8 Dci; > UINT8 MaxDci; > @@ -3074,7 +3077,8 @@ XhcInitializeEndpointContext64 ( > InputContext->EP[Dci-1].MaxBurstSize = 0x0; > } > > - switch (EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK) { > + EpType = EpDesc->Attributes & USB_ENDPOINT_TYPE_MASK; > + switch (EpType) { > case USB_ENDPOINT_BULK: > if (Direction == EfiUsbDataIn) { > InputContext->EP[Dci-1].CErr = 3; > @@ -3177,13 +3181,14 @@ XhcInitializeEndpointContext64 ( > > break; > > - case USB_ENDPOINT_CONTROL: > - // > - // Do not support control transfer now. > - // > - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unsupport Control EP found, Transfer ring is not allocated.\n")); > default: > - DEBUG ((DEBUG_INFO, "XhcInitializeEndpointContext64: Unknown EP found, Transfer ring is not allocated.\n")); > + DEBUG (( > + DEBUG_INFO, > + "%a: %a found, Transfer ring is not allocated.\n", > + __func__, > + ((EpType == USB_ENDPOINT_CONTROL) ? "Unsupported Control EP" : "Unknown EP") > + )); > + > EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length); > continue; > } Reviewed-by: Laszlo Ersek <lersek@redhat.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#111167): https://edk2.groups.io/g/devel/message/111167 Mute This Topic: https://groups.io/mt/102502056/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-13 17:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-11-10 5:22 [edk2-devel] [PATCH v3 0/2] BZ 4221: Fix MdeModulePkg/Bus/Pci/XhciDxe issues pointed by Coverity Ranbir Singh 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 1/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix FORWARD_NULL Coverity issues Ranbir Singh 2023-11-13 16:57 ` Laszlo Ersek 2023-11-10 5:22 ` [edk2-devel] [PATCH v3 2/2] MdeModulePkg/Bus/Pci/XhciDxe: Fix MISSING_BREAK " Ranbir Singh 2023-11-13 17:00 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox