From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0250321DF806E for ; Tue, 29 Aug 2017 08:59:45 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 069B834CD; Tue, 29 Aug 2017 16:02:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 069B834CD Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-166.phx2.redhat.com [10.3.116.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id 735A9692AE; Tue, 29 Aug 2017 16:02:22 +0000 (UTC) To: Brijesh Singh , edk2-devel@lists.01.org Cc: Jordan Justen , Tom Lendacky , Ard Biesheuvel References: <1503919610-26185-1-git-send-email-brijesh.singh@amd.com> <1503919610-26185-3-git-send-email-brijesh.singh@amd.com> From: Laszlo Ersek Message-ID: <9f3256f3-6acd-e88b-7fb4-dccb594d6926@redhat.com> Date: Tue, 29 Aug 2017 18:02:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <1503919610-26185-3-git-send-email-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Tue, 29 Aug 2017 16:02:25 +0000 (UTC) Subject: Re: [PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Aug 2017 15:59:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Jordan Justen > Cc: Tom Lendacky > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Brijesh Singh > --- > 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; > } > > >