Re: [PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers

[Laszlo Ersek <lersek@redhat.com>]

Hi Brijesh,

On 08/28/17 13:26, Brijesh Singh wrote:
> When device is behind the IOMMU, driver is require to pass the device
> address of virtio request, response and any memory referenced by those
> request/response to the bus master.
>
> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to
> map request and response buffers system physical address to the device
> address.
>
> - If the buffer need to be accessed by both the processor and a bus
>   master then map with BusMasterCommonBuffer.
>
> - If the buffer need to be accessed for a write operation by a bus master
>   then map with BusMasterWrite.
>
> - If the buffer need to be accessed for a read  operation by a bus master
>   then map with BusMasterRead.
>
> 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/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++--
>  1 file changed, 152 insertions(+), 16 deletions(-)

I have several comments here. I believe that this time my comments are
best kept together, so I'm not going to scatter them all over the patch.

(1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi"
instead.


(2) "ResponseBuffer" is not adequately zeroed before it is mapped. The
Response->Response field is set correctly, but the entire structure
should be zeroed, after it is allocated. (Practically in place of the
ZeroMem() that you remove at the top of the patch.)

For VirtioBlkDxe, this wasn't an issue, because there the entire
response buffer was a single byte, "HostStatus". By setting that byte,
no stale data was left in the area that would be exposed to the host.


(3) By reviewing this patch, I realized that I missed an error in commit
3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response
buffers", 2017-08-27).

In other words, this point is about the VirtioBlkDxe driver.

Namely, 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 critical -- we must not do that unmapping on the error path only. If
the unmapping fails, then the device's data don't reach the caller, and
we must fail the request with EFI_DEVICE_ERROR.

I believe the mitigation could be:

> 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'm very sorry about this. If you agree with the above suggestion, can
you please submit it as a standalone patch?


(4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru()
member function is required to implement elaborate error reporting, and
some output fields must be set even if we report EFI_DEVICE_ERROR. The
pre-patch code conforms to the UEFI spec's requirements, and we should
keep that up.

Please locate the following code:

  // If kicking the host fails, we must fake a host adapter error.
  // EFI_NOT_READY would save us the effort, but it would also suggest that the
  // caller retry.
  //
  if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring,
        &Indices, NULL) != EFI_SUCCESS) {
    Packet->InTransferLength  = 0;
    Packet->OutTransferLength = 0;
    Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
    Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
    Packet->SenseDataLength   = 0;
    return EFI_DEVICE_ERROR;
  }

This entire block of code should be factored out to a helper function,
in a separate patch:

STATIC
EFI_STATUS
ReportHostAdapterError (
  OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet
  )
{
  Packet->InTransferLength  = 0;
  Packet->OutTransferLength = 0;
  Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
  Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
  Packet->SenseDataLength   = 0;
  return EFI_DEVICE_ERROR;
}

Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR
from *before* VirtioFlush(), do:

  /* attempt shared pages allocation or mapping, as appropriate */
  /* ... */
  if (EFI_ERROR (Status)) {
    Status = ReportHostAdapterError (Packet);
    goto ErrorHandlingLabel;
  }

(The same should also be performed when VirtioFlush() itself fails, of
course.)

The idea is, if PopulateRequest() succeeds, but we don't reach
VirtioFlush() for another -- new -- error scenario, or VirtioFlush()
itself fails (like before), then we must fake this kind of host adapter
error in *all* cases. The above helper function will simplify that.


(5) The same issue as (3) is present in this patch (i.e., for
VirtioScsiDxe); however, it is more complicated here.

Assume that

- the caller submits a bi-directional request,

- we actually perform the request fine,

- but then we fail to unmap "InDataMapping".

In this case, flipping the return status to EFI_DEVICE_ERROR just
doesn't cut it: we'd have to update the Packet->xxxx fields accordingly,
so that they report the full loss of the incoming transfer. This would
be hellishly difficult; the update would have to be different for a
bidirectional transfer vs. for a simple read, and in general, who wants
to mess with SCSI sense data?

Therefore we have to guarantee *in advance* that this error won't
present itself. Which means, of course, a CommonBuffer mapping for the
input transfer (if any):

(5a) If "Packet->InTransferLength" is positive on input, allocate a
shared buffer, zero it (!), and finally map it for CommonBuffer
operation (--> InDataMapping). Proceed with the rest of the patch.

(5b) If VirtioFlush() fails, then do as before (see point (4) above) --
report a host adapter error.

(5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its
return value in "Status".

(5d) Now, ParseResponse() will *always* update
"Packet->InTransferLength". Therefore, after ParseResponse() returns, if
"Packet->InTransferLength" is positive, then we have to CopyMem()
"Packet->InTransferLength" bytes from the common buffer to
"Packet->InDataBuffer".

This way, we'll only have to unmap BusMasterCommonBuffer and
BusMasterRead mappings on the final path (both on success and error),
and we won't have to care about their return values.

Also, on the final path (on both success and error), we don't have to
touch "Packet":

- we either reached ParseResponse(), and then it set the output fields
  appropriately,

- or we jumped over ParseResponse() due to an error, but in all such
  cases, we called ReportHostAdapterError(), so the output fields are
  set again.

Thank you,
Laszlo


On 08/28/17 13:26, Brijesh Singh wrote:

> diff --git a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> index 5e72b1a24b59..a1ee810919e4 100644
> --- a/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> +++ b/OvmfPkg/VirtioScsiDxe/VirtioScsi.c
> @@ -409,11 +409,19 @@ VirtioScsiPassThru (
>    UINT16                    TargetValue;
>    EFI_STATUS                Status;
>    volatile VIRTIO_SCSI_REQ  Request;
> -  volatile VIRTIO_SCSI_RESP Response;
> +  volatile VIRTIO_SCSI_RESP *Response;
> +  VOID                      *ResponseBuffer;
>    DESC_INDICES              Indices;
> +  VOID                      *RequestMapping;
> +  VOID                      *ResponseMapping;
> +  VOID                      *InDataMapping;
> +  VOID                      *OutDataMapping;
> +  EFI_PHYSICAL_ADDRESS      RequestDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      ResponseDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      InDataDeviceAddress;
> +  EFI_PHYSICAL_ADDRESS      OutDataDeviceAddress;
>
>    ZeroMem ((VOID*) &Request, sizeof (Request));
> -  ZeroMem ((VOID*) &Response, sizeof (Response));
>
>    Dev = VIRTIO_SCSI_FROM_PASS_THRU (This);
>    CopyMem (&TargetValue, Target, sizeof TargetValue);
> @@ -423,12 +431,96 @@ VirtioScsiPassThru (
>      return Status;
>    }
>
> -  VirtioPrepare (&Dev->Ring, &Indices);
> +  //
> +  // Map the scsi-blk Request header buffer
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterRead,
> +             (VOID *) &Request,
> +             sizeof Request,
> +             &RequestDeviceAddress,
> +             &RequestMapping);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_DEVICE_ERROR;
> +  }
> +
> +  //
> +  // Map the input buffer
> +  //
> +  if (Packet->InTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterWrite,
> +               Packet->InDataBuffer,
> +               Packet->InTransferLength,
> +               &InDataDeviceAddress,
> +               &InDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapRequestBuffer;
> +    }
> +  }
> +
> +  //
> +  // Map the output buffer
> +  //
> +  if (Packet->OutTransferLength > 0) {
> +    Status = VirtioMapAllBytesInSharedBuffer (
> +               Dev->VirtIo,
> +               VirtioOperationBusMasterRead,
> +               Packet->OutDataBuffer,
> +               Packet->OutTransferLength,
> +               &OutDataDeviceAddress,
> +               &OutDataMapping
> +               );
> +    if (EFI_ERROR (Status)) {
> +      Status = EFI_DEVICE_ERROR;
> +      goto UnmapInDataBuffer;
> +    }
> +  }
> +
> +  //
> +  // Response header is bi-direction (we preset with host status and expect the
> +  // device to update it). Allocate a response buffer which can be mapped to
> +  // access equally by both processor and device.
> +  //
> +  Status = Dev->VirtIo->AllocateSharedPages (
> +                          Dev->VirtIo,
> +                          EFI_SIZE_TO_PAGES (sizeof *Response),
> +                          &ResponseBuffer
> +                          );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapOutDataBuffer;
> +  }
> +
> +  Response = ResponseBuffer;
>
>    //
>    // preset a host status for ourselves that we do not accept as success
>    //
> -  Response.Response = VIRTIO_SCSI_S_FAILURE;
> +  Response->Response = VIRTIO_SCSI_S_FAILURE;
> +
> +  //
> +  // Map the response buffer with BusMasterCommonBuffer so that response
> +  // buffer can be accessed by both host and device.
> +  //
> +  Status = VirtioMapAllBytesInSharedBuffer (
> +             Dev->VirtIo,
> +             VirtioOperationBusMasterCommonBuffer,
> +             ResponseBuffer,
> +             sizeof (*Response),
> +             &ResponseDeviceAddress,
> +             &ResponseMapping
> +             );
> +  if (EFI_ERROR (Status)) {
> +    Status = EFI_DEVICE_ERROR;
> +    goto FreeResponseBuffer;
> +  }
> +
> +  VirtioPrepare (&Dev->Ring, &Indices);
>
>    //
>    // ensured by VirtioScsiInit() -- this predicate, in combination with the
> @@ -439,31 +531,49 @@ VirtioScsiPassThru (
>    //
>    // enqueue Request
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Request, sizeof Request,
> -    VRING_DESC_F_NEXT, &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    RequestDeviceAddress,
> +    sizeof Request,
> +    VRING_DESC_F_NEXT,
> +    &Indices
> +    );
>
>    //
>    // enqueue "dataout" if any
>    //
>    if (Packet->OutTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->OutDataBuffer,
> -      Packet->OutTransferLength, VRING_DESC_F_NEXT, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      OutDataDeviceAddress,
> +      Packet->OutTransferLength,
> +      VRING_DESC_F_NEXT,
> +      &Indices
> +      );
>    }
>
>    //
>    // enqueue Response, to be written by the host
>    //
> -  VirtioAppendDesc (&Dev->Ring, (UINTN) &Response, sizeof Response,
> -    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ?
> -                          VRING_DESC_F_NEXT : 0),
> -    &Indices);
> +  VirtioAppendDesc (
> +    &Dev->Ring,
> +    ResponseDeviceAddress,
> +    sizeof *Response,
> +    VRING_DESC_F_WRITE | (Packet->InTransferLength > 0 ? VRING_DESC_F_NEXT : 0),
> +    &Indices
> +    );
>
>    //
>    // enqueue "datain" if any, to be written by the host
>    //
>    if (Packet->InTransferLength > 0) {
> -    VirtioAppendDesc (&Dev->Ring, (UINTN) Packet->InDataBuffer,
> -      Packet->InTransferLength, VRING_DESC_F_WRITE, &Indices);
> +    VirtioAppendDesc (
> +      &Dev->Ring,
> +      InDataDeviceAddress,
> +      Packet->InTransferLength,
> +      VRING_DESC_F_WRITE,
> +      &Indices
> +      );
>    }
>
>    // If kicking the host fails, we must fake a host adapter error.
> @@ -477,10 +587,36 @@ VirtioScsiPassThru (
>      Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER;
>      Packet->TargetStatus      = EFI_EXT_SCSI_STATUS_TARGET_GOOD;
>      Packet->SenseDataLength   = 0;
> -    return EFI_DEVICE_ERROR;
> +    Status = EFI_DEVICE_ERROR;
> +    goto UnmapResponseBuffer;
>    }
>
> -  return ParseResponse (Packet, &Response);
> +  Status = ParseResponse (Packet, Response);
> +
> +UnmapResponseBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, ResponseMapping);
> +
> +FreeResponseBuffer:
> +  Dev->VirtIo->FreeSharedPages (
> +                 Dev->VirtIo,
> +                 EFI_SIZE_TO_PAGES (sizeof *Response),
> +                 ResponseBuffer
> +                 );
> +
> +UnmapOutDataBuffer:
> +  if (Packet->OutTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, OutDataMapping);
> +  }
> +
> +UnmapInDataBuffer:
> +  if (Packet->InTransferLength > 0) {
> +    Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, InDataMapping);
> +  }
> +
> +UnmapRequestBuffer:
> +  Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, RequestMapping);
> +
> +  return Status;
>  }
>
>
>