From 1fc55a3933b0d17430c2857629ee54abefaad7eb Mon Sep 17 00:00:00 2001
From: Sami Mujawar <sami.mujawar@arm.com>
Date: Mon, 15 Apr 2024 09:51:13 +0100
Subject: OvmfPkg: Use heap memory for virtio-scsi request

The storage space for virtio-scsi request header being shared
with the host was from the stack as the request structure was
a local function variable.

A bug in the VMM can corrupt the stack space, and such issues
can be very hard to debug.

Note: This is only an issue with a normal guest VM (non-CCA).
A CCA guest VM would perform bounce buffering for sharing the
data and therefore not have this issue.

Instead of using the stack for sharing the data with the host,
memory can be allocated from the heap pool. However, pool
allocations are not any safer in terms of pages being shared
between different allocations, and so mapping a pool allocation
for DMA may expose it to potential corruption by the VMM in
exactly the same way. The only difference is the potential
impact on program behaviour, which is much higher with the
stack.

Additionally, for guest-side corruption heap allocations can
take advantage by turning on heap guard to help find the bug.

Therefore, minor improvement can be achieved by allocating
memory for the virtio-scsi request header from the heap for
sharing with the host.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
 OvmfPkg/VirtioScsiDxe/VirtioScsi.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

(limited to 'OvmfPkg')

diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
index 3705f5fc14..11e4d8703d 100644
--- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
+++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
@@ -28,6 +28,7 @@
   Copyright (C) 2012, Red Hat, Inc.
   Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
+  Copyright (c) 2024, Arm Limited. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -430,7 +431,7 @@ VirtioScsiPassThru (
   VSCSI_DEV                  *Dev;
   UINT16                     TargetValue;
   EFI_STATUS                 Status;
-  volatile VIRTIO_SCSI_REQ   Request;
+  volatile VIRTIO_SCSI_REQ   *Request;
   volatile VIRTIO_SCSI_RESP  *Response;
   VOID                       *ResponseBuffer;
   DESC_INDICES               Indices;
@@ -455,7 +456,10 @@ VirtioScsiPassThru (
   InDataDeviceAddress  = 0;
   OutDataDeviceAddress = 0;
 
-  ZeroMem ((VOID *)&Request, sizeof (Request));
+  Request = AllocateZeroPool (sizeof (*Request));
+  if (Request == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
 
   Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
   CopyMem (&TargetValue, Target, sizeof TargetValue);
@@ -464,9 +468,9 @@ VirtioScsiPassThru (
   OutDataBufferIsMapped = FALSE;
   InDataNumPages        = 0;
 
-  Status = PopulateRequest (Dev, TargetValue, Lun, Packet, &Request);
+  Status = PopulateRequest (Dev, TargetValue, Lun, Packet, Request);
   if (EFI_ERROR (Status)) {
-    return Status;
+    goto FreeScsiRequest;
   }
 
   //
@@ -475,13 +479,14 @@ VirtioScsiPassThru (
   Status = VirtioMapAllBytesInSharedBuffer (
              Dev->VirtIo,
              VirtioOperationBusMasterRead,
-             (VOID *)&Request,
-             sizeof Request,
+             (VOID *)Request,
+             sizeof (*Request),
              &RequestDeviceAddress,
              &RequestMapping
              );
   if (EFI_ERROR (Status)) {
-    return ReportHostAdapterError (Packet);
+    Status = ReportHostAdapterError (Packet);
+    goto FreeScsiRequest;
   }
 
   //
@@ -702,6 +707,9 @@ FreeInDataBuffer:
 UnmapRequestBuffer:
   Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
 
+FreeScsiRequest:
+  FreePool ((VOID *)Request);
+
   return Status;
 }
 
-- 
cgit v1.2.3