MdeModulePkg/SdMmcPciHcDxe: use FreeBuffer not FreePages from DMA buffer

[edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: use FreeBuffer not FreePages from DMA buffer

Posted by Ard Biesheuvel 7 years, 1 month ago

Don't use EFI_PCI_IO_PROTOCOL::FreePages () to free an allocation
created with EFI_PCI_IO_PROTOCOL::AllocatePages (). It is simply
incorrect, but given that it may interfere with IOMMU DMA protection
and/or memory encryption, it could pose a security risk as well.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 23faec5e2be0..a6005dd33bce 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -1009,7 +1009,7 @@ SdMmcPassThruPassThru (
 
 Done:
   if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
-    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
+    Private->PciIo->FreeBuffer (Private->PciIo, Trb->AdmaPages, Trb->AdmaDesc);
   }
 
   if (Trb != NULL) {
-- 
2.11.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/SdMmcPciHcDxe: use FreeBuffer not FreePages from DMA buffer

Posted by Ard Biesheuvel 7 years, 1 month ago

On 4 November 2017 at 21:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Don't use EFI_PCI_IO_PROTOCOL::FreePages () to free an allocation
> created with EFI_PCI_IO_PROTOCOL::AllocatePages (). It is simply
> incorrect, but given that it may interfere with IOMMU DMA protection
> and/or memory encryption, it could pose a security risk as well.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 23faec5e2be0..a6005dd33bce 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -1009,7 +1009,7 @@ SdMmcPassThruPassThru (
>
>  Done:
>    if ((Trb != NULL) && (Trb->AdmaDesc != NULL)) {
> -    FreePages (Trb->AdmaDesc, Trb->AdmaPages);
> +    Private->PciIo->FreeBuffer (Private->PciIo, Trb->AdmaPages, Trb->AdmaDesc);

Actually, looking a bit deeper, it seems we need to simply call
SdMmcFreeTrb () here, otherwise PciIo->Unmap() is never called on the
DataMap mapping, which breaks non-coherent DMA (and also coherent
bounce buffering DMA)


>    }
>
>    if (Trb != NULL) {
> --
> 2.11.0
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel