Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MdeModulePkg/Bus/Pci: Fix Descriptor Misalignment in USB Config Handling #6442

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MdeModulePkg/Bus/Pci: Fix Descriptor Misalignment in USB Config Handling #6442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2848,7 +2848,7 @@ XhcInitializeEndpointContext (
MaxDci = 1;
}

EpDesc = (USB_ENDPOINT_DESCRIPTOR *)(IfDesc + 1);
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since IfDesc and ConfigDesc are pointer values, it would be better to type cast to another pointer type such as UINT8 * instead of an integer type of UINTN.

Copy link
Author

@AniketSurekar AniketSurekar Nov 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had a similar thought, but another file with existing code uses UINTN typecasting for IfDesc , ConfigDesc and EpDesc, so I kept it the same to maintain consistency. I referenced some of the existing code in
edk2/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c:

XhcInitializeEndpointContext() API: lines 2854, 2858, 2935, 2993, 3008
XhcInitializeEndpointContext64() API: lines 3057, 3061, 3138, 3196, 3211 And so on....

Similarly, in edk2/MdeModulePkg/Bus/Pci/XhciPei/XhciSched.c:

XhcPeiSetConfigCmd API: lines 1765, 1833, 1880, 1895
XhcPeiSetConfigCmd64 API: lines 1991, 2060, 2107, 2124 And so on....

For uniformity, I chose to retain the same typecasting.

Copy link
Author

@AniketSurekar AniketSurekar Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdkinney @lgao4 @niruiyu If you are satisfied with the changes, could you please approve them?

These changes address an issue where multiple third-party EFTPOS USB devices caused the system to hang in preboot after being connected to the SUT.

Detail analysis:

Analysis of the Issue: (same analysis is applicable for End point descriptor)
The issue occurs because the Interface/Endpoint Descriptor does not receive the expected value, and the length of the interface descriptor is being read as 0 (9th byte of data is 0x0 mentioned in hex dump snap). Consequently, the system hangs in a while loop.
image

The root cause is that the configDesc (USB_CONFIG_DESCRIPTOR) structure has a fixed length of 9 bytes. However, recent logs from the dump show that a third-party USB device reports a configuration length of 0x0A (10 bytes). In the code, the length is incremented by 9 bytes, causing the interface descriptor to be incorrectly updated from 9 bytes instead of 10 bytes. This mismatch results in the interface descriptor and End Point descriptor type and length being incorrect.

Snap hex dump of config/interface/end point descriptor (Red highlighted config descriptor , Blue highlighted Interface descriptor and remaining dump for end point descriptor )
image

So fix as mentioned :
Update the code to increment the pointers by the actual descriptor lengths,
ensuring proper access to all descriptors in the USB configuration.

image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdkinney @lgao4 @niruiyu If you are satisfied with the changes, could you please approve them?

for (EpIndex = 0; EpIndex < NumEp; EpIndex++) {
while (EpDesc->DescriptorType != USB_DESC_TYPE_ENDPOINT) {
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
Expand Down Expand Up @@ -3051,7 +3051,7 @@ XhcInitializeEndpointContext64 (
MaxDci = 1;
}

EpDesc = (USB_ENDPOINT_DESCRIPTOR *)(IfDesc + 1);
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
for (EpIndex = 0; EpIndex < NumEp; EpIndex++) {
while (EpDesc->DescriptorType != USB_DESC_TYPE_ENDPOINT) {
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
Expand Down Expand Up @@ -3260,7 +3260,7 @@ XhcSetConfigCmd (

MaxDci = 0;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
for (Index = 0; Index < ConfigDesc->NumInterfaces; Index++) {
while ((IfDesc->DescriptorType != USB_DESC_TYPE_INTERFACE) || (IfDesc->AlternateSetting != 0)) {
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
Expand Down Expand Up @@ -3353,7 +3353,7 @@ XhcSetConfigCmd64 (

MaxDci = 0;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
for (Index = 0; Index < ConfigDesc->NumInterfaces; Index++) {
while ((IfDesc->DescriptorType != USB_DESC_TYPE_INTERFACE) || (IfDesc->AlternateSetting != 0)) {
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
Expand Down Expand Up @@ -3644,7 +3644,7 @@ XhcSetInterface (
IfDescActive = NULL;
IfDescSet = NULL;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
while ((UINTN)IfDesc < ((UINTN)ConfigDesc + ConfigDesc->TotalLength)) {
if ((IfDesc->DescriptorType == USB_DESC_TYPE_INTERFACE) && (IfDesc->Length >= sizeof (USB_INTERFACE_DESCRIPTOR))) {
if (IfDesc->InterfaceNumber == (UINT8)Request->Index) {
Expand Down Expand Up @@ -3851,7 +3851,7 @@ XhcSetInterface64 (
IfDescActive = NULL;
IfDescSet = NULL;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
while ((UINTN)IfDesc < ((UINTN)ConfigDesc + ConfigDesc->TotalLength)) {
if ((IfDesc->DescriptorType == USB_DESC_TYPE_INTERFACE) && (IfDesc->Length >= sizeof (USB_INTERFACE_DESCRIPTOR))) {
if (IfDesc->InterfaceNumber == (UINT8)Request->Index) {
Expand Down
8 changes: 4 additions & 4 deletions MdeModulePkg/Bus/Pci/XhciPei/XhciSched.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1748,7 +1748,7 @@ XhcPeiSetConfigCmd (

MaxDci = 0;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
for (Index = 0; Index < ConfigDesc->NumInterfaces; Index++) {
while ((IfDesc->DescriptorType != USB_DESC_TYPE_INTERFACE) || (IfDesc->AlternateSetting != 0)) {
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
Expand All @@ -1759,7 +1759,7 @@ XhcPeiSetConfigCmd (
MaxDci = 1;
}

EpDesc = (USB_ENDPOINT_DESCRIPTOR *)(IfDesc + 1);
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
for (EpIndex = 0; EpIndex < NumEp; EpIndex++) {
while (EpDesc->DescriptorType != USB_DESC_TYPE_ENDPOINT) {
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
Expand Down Expand Up @@ -1974,7 +1974,7 @@ XhcPeiSetConfigCmd64 (

MaxDci = 0;

IfDesc = (USB_INTERFACE_DESCRIPTOR *)(ConfigDesc + 1);
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)ConfigDesc + ConfigDesc->Length);
for (Index = 0; Index < ConfigDesc->NumInterfaces; Index++) {
while ((IfDesc->DescriptorType != USB_DESC_TYPE_INTERFACE) || (IfDesc->AlternateSetting != 0)) {
IfDesc = (USB_INTERFACE_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
Expand All @@ -1985,7 +1985,7 @@ XhcPeiSetConfigCmd64 (
MaxDci = 1;
}

EpDesc = (USB_ENDPOINT_DESCRIPTOR *)(IfDesc + 1);
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)IfDesc + IfDesc->Length);
for (EpIndex = 0; EpIndex < NumEp; EpIndex++) {
while (EpDesc->DescriptorType != USB_DESC_TYPE_ENDPOINT) {
EpDesc = (USB_ENDPOINT_DESCRIPTOR *)((UINTN)EpDesc + EpDesc->Length);
Expand Down
Loading