It would be possible to remove the UAF without local variables, by calling SataPrivateData->PciIo->Attributes() before releasing SataPrivateData.
However, by keeping the location of the call (for which temporary variables are necessary), we continue to match the error path logic in SataControllerStart(), which is always recommended. Reported-by: wang xiaofeng <[email protected]> Fixes: bcab71413407e61c144994925556725dd65eede9 Cc: wang xiaofeng <[email protected]> Cc: Jordan Justen <[email protected]> Cc: Ruiyu Ni <[email protected]> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <[email protected]> --- OvmfPkg/SataControllerDxe/SataController.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/OvmfPkg/SataControllerDxe/SataController.c b/OvmfPkg/SataControllerDxe/SataController.c index e5ee63a0ab63..1f84ad034e5b 100644 --- a/OvmfPkg/SataControllerDxe/SataController.c +++ b/OvmfPkg/SataControllerDxe/SataController.c @@ -563,90 +563,95 @@ EFIAPI SataControllerStop ( IN EFI_DRIVER_BINDING_PROTOCOL *This, IN EFI_HANDLE Controller, IN UINTN NumberOfChildren, IN EFI_HANDLE *ChildHandleBuffer ) { EFI_STATUS Status; EFI_IDE_CONTROLLER_INIT_PROTOCOL *IdeInit; EFI_SATA_CONTROLLER_PRIVATE_DATA *SataPrivateData; + EFI_PCI_IO_PROTOCOL *PciIo; + UINT64 OriginalPciAttributes; // // Open the produced protocol // Status = gBS->OpenProtocol ( Controller, &gEfiIdeControllerInitProtocolGuid, (VOID **) &IdeInit, This->DriverBindingHandle, Controller, EFI_OPEN_PROTOCOL_GET_PROTOCOL ); if (EFI_ERROR (Status)) { return EFI_UNSUPPORTED; } SataPrivateData = SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS (IdeInit); ASSERT (SataPrivateData != NULL); + PciIo = SataPrivateData->PciIo; + OriginalPciAttributes = SataPrivateData->OriginalPciAttributes; + // // Uninstall the IDE Controller Init Protocol from this instance // Status = gBS->UninstallMultipleProtocolInterfaces ( Controller, &gEfiIdeControllerInitProtocolGuid, &(SataPrivateData->IdeInit), NULL ); if (EFI_ERROR (Status)) { return Status; } if (SataPrivateData->DisqualifiedModes != NULL) { FreePool (SataPrivateData->DisqualifiedModes); } if (SataPrivateData->IdentifyData != NULL) { FreePool (SataPrivateData->IdentifyData); } if (SataPrivateData->IdentifyValid != NULL) { FreePool (SataPrivateData->IdentifyValid); } FreePool (SataPrivateData); // // Restore original PCI attributes // - SataPrivateData->PciIo->Attributes ( - SataPrivateData->PciIo, - EfiPciIoAttributeOperationSet, - SataPrivateData->OriginalPciAttributes, - NULL - ); + PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSet, + OriginalPciAttributes, + NULL + ); // // Close protocols opened by Sata Controller driver // return gBS->CloseProtocol ( Controller, &gEfiPciIoProtocolGuid, This->DriverBindingHandle, Controller ); } /** Calculate the flat array subscript of a (Channel, Device) pair. @param[in] SataPrivateData The private data structure corresponding to the SATA controller that attaches the device for which the flat array subscript is being calculated. @param[in] Channel The channel (ie. port) number on the SATA controller that the device is attached to. @param[in] Device The device number on the channel. @return The flat array subscript suitable for indexing DisqualifiedModes, IdentifyData, and IdentifyValid. **/ -- 1.8.3.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
