[PATCH 1/1] OvmfPkg/VirtioBlk: Check the returns status of unmap data buffer

[PATCH 1/1] OvmfPkg/VirtioBlk: Check the returns status of unmap data buffer

[Brijesh Singh]

when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
the device, we map "Buffer" for VirtioOperationBusMasterWrite. In
this case, checking the return status of

Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);

is must. If the unmapping fails, then "Buffer" will not contain the
actual data from the device, and we must fail the request with
EFI_DEVICE_ERROR.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
index 6abd937f86c6..5a63986b3f39 100644
--- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
+++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
@@ -260,6 +260,7 @@ SynchronousRequest (
   EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
   EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
   EFI_STATUS              Status;
+  EFI_STATUS              UnmapStatus;
 
   BlockSize = Dev->BlockIoMedia.BlockSize;
 
@@ -430,7 +431,13 @@ SynchronousRequest (
 
 UnmapDataBuffer:
   if (BufferSize > 0) {
-    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
+    UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
+    if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
+      //
+      // Data from the bus master may not reach the caller; fail the request.
+      //
+      Status = EFI_DEVICE_ERROR;
+    }
   }
 
 UnmapRequestBuffer:
-- 
2.7.4

Re: [PATCH 1/1] OvmfPkg/VirtioBlk: Check the returns status of unmap data buffer

[Laszlo Ersek]

On 08/30/17 18:28, Brijesh Singh wrote:
> when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from
> the device, we map "Buffer" for VirtioOperationBusMasterWrite. In
> this case, checking the return status of
> 
> Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> 
> is must. If the unmapping fails, then "Buffer" will not contain the
> actual data from the device, and we must fail the request with
> EFI_DEVICE_ERROR.
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> index 6abd937f86c6..5a63986b3f39 100644
> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c
> @@ -260,6 +260,7 @@ SynchronousRequest (
>    EFI_PHYSICAL_ADDRESS    HostStatusDeviceAddress;
>    EFI_PHYSICAL_ADDRESS    RequestDeviceAddress;
>    EFI_STATUS              Status;
> +  EFI_STATUS              UnmapStatus;
>  
>    BlockSize = Dev->BlockIoMedia.BlockSize;
>  
> @@ -430,7 +431,13 @@ SynchronousRequest (
>  
>  UnmapDataBuffer:
>    if (BufferSize > 0) {
> -    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +    UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping);
> +    if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) {
> +      //
> +      // Data from the bus master may not reach the caller; fail the request.
> +      //
> +      Status = EFI_DEVICE_ERROR;
> +    }
>    }
>  
>  UnmapRequestBuffer:
> 

I slightly corrected the subject:

-OvmfPkg/VirtioBlk: Check the returns status of unmap data buffer
+OvmfPkg/VirtioBlkDxe: Check the return status of unmap data buffer

I tested this patch in the following environments:
- no SEV, legacy virtio, X64: PASS
- no SEV, modern virtio, X64: PASS
- SEV, modern virtio: PASS

[lersek@redhat.com: fix typos in subject]
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

Pushed as commit ea8314e4402f.

Thanks!
Laszlo