MdeModulePkg: XhciDxe: Fix USB reset issue: use after free

`XhcFreeUrb` attempts to unmap `Urb->DataMap` after
`FreePool(Urb->Data)` has already been called in some cases, causing a
use after free. Change the ordering so `Data` is freed after `Unmap`and
only freed when appropriate.

Signed-off-by: Sherry Fan <sherryfan@microsoft.com>

2 files changed, 33 insertions, 4 deletions

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
index c830db66a1..c337f15118 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/Xhci.c
@@ -837,6 +837,10 @@ XhcTransfer (
   }

 

   Xhc->PciIo->Flush (Xhc->PciIo);

+  //

+  // Do not free URB data, since it is passed in as an external argument

+  // and allocated and managed by the caller.

+  //

   XhcFreeUrb (Xhc, Urb);

   return Status;

 }

@@ -1227,8 +1231,9 @@ ON_EXIT:
                                 sending or receiving.

   @param  DataBuffersNumber     Number of data buffers prepared for the transfer.

   @param  Data                  Array of pointers to the buffers of data to transmit

-                                from or receive into.

-  @param  DataLength            The lenght of the data buffer.

+                                from or receive into. The caller is responsible for freeing

+                                the buffers after the transfers are completed.

+  @param  DataLength            The length of the data buffer.

   @param  DataToggle            On input, the initial data toggle for the transfer;

                                 On output, it is updated to to next data toggle to

                                 use of the subsequent bulk transfer.

diff --git a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
index 50d7d4b5f4..e0171d6699 100644
--- a/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
+++ b/MdeModulePkg/Bus/Pci/XhciDxe/XhciSched.c
@@ -107,6 +107,10 @@ XhcCmdTransfer (
     Status = EFI_SUCCESS;

   }

 

+  //

+  // Do not free URB data, since `XhcCreateCmdTrb` does not allocate any data

+  // and the `Data` field is not used in command transfers.

+  //

   XhcFreeUrb (Xhc, Urb);

 

 ON_EXIT:

@@ -184,6 +188,9 @@ XhcCreateUrb (
 

 /**

   Free an allocated URB.

+  The `Data` field of the URB is not owned by the URB and is not freed here.

+  The caller is which allocates `Data` is responsible for freeing it.

+  Freeing `Data` must be done AFTER calling `XhcFreeUrb`, since this function may unmap the `DataMap` field.

 

   @param  Xhc                   The XHCI device.

   @param  Urb                   The URB to free.

@@ -1388,6 +1395,7 @@ XhciDelAsyncIntTransfer (
   LIST_ENTRY              *Entry;

   LIST_ENTRY              *Next;

   URB                     *Urb;

+  VOID                    *UrbData;

   EFI_USB_DATA_DIRECTION  Direction;

   EFI_STATUS              Status;

 

@@ -1412,8 +1420,16 @@ XhciDelAsyncIntTransfer (
       }

 

       RemoveEntryList (&Urb->UrbList);

-      FreePool (Urb->Data);

+      //

+      // For `XhciDelAsyncIntTransfer`, the URB is created through `XhciInsertAsyncIntTransfer`

+      // and allocates and manages its own data buffer, so free it here.

+      //

+      UrbData = Urb->Data;

       XhcFreeUrb (Xhc, Urb);

+      if (UrbData != NULL) {

+        FreePool (UrbData);

+      }

+

       return EFI_SUCCESS;

     }

   }

@@ -1435,6 +1451,7 @@ XhciDelAllAsyncIntTransfers (
   LIST_ENTRY  *Entry;

   LIST_ENTRY  *Next;

   URB         *Urb;

+  VOID        *UrbData;

   EFI_STATUS  Status;

 

   BASE_LIST_FOR_EACH_SAFE (Entry, Next, &Xhc->AsyncIntTransfers) {

@@ -1450,8 +1467,15 @@ XhciDelAllAsyncIntTransfers (
     }

 

     RemoveEntryList (&Urb->UrbList);

-    FreePool (Urb->Data);

+    //

+    // For `XhciDelAllAsyncIntTransfers`, the URB is created through `XhciInsertAsyncIntTransfer`

+    // and allocates and manages its own data buffer, so free it here.

+    //

+    UrbData = Urb->Data;

     XhcFreeUrb (Xhc, Urb);

+    if (UrbData != NULL) {

+      FreePool (UrbData);

+    }

   }

 }