[PATCH V2 1/1] x86/VMBus: Confidential VMBus for dynamic DMA transfers

[PATCH V2 1/1] x86/VMBus: Confidential VMBus for dynamic DMA transfers

[Tianyu Lan]

Hyper-V provides Confidential VMBus to communicate between
device model and device guest driver via encrypted/private
memory in Confidential VM. The device model is in OpenHCL
(https://openvmm.dev/guide/user_guide/openhcl.html) that
plays the paravisor role.

For a VMBus device, there are two communication methods to
talk with Host/Hypervisor. 1) VMBUS Ring buffer 2) Dynamic
DMA transfer.

The Confidential VMBus Ring buffer has been upstreamed by
Roman Kisel(commit 6802d8af47d1).

The dynamic DMA transition of VMBus device normally goes
through DMA core and it uses SWIOTLB as bounce buffer in
a CoCo VM.

The Confidential VMBus device can do DMA directly to
private/encrypted memory. Because the swiotlb is decrypted
memory, the DMA transfer must not be bounced through the
swiotlb, so as to preserve confidentiality. This is different
from the default for Linux CoCo VMs, so not use DMA(SWIOTLB)
API in VMBus driver when confidential dynamic DMA transfers
capability is present.

Signed-off-by: Tianyu Lan <tiala@microsoft.com>
---
Change since v1:
	* Fix code style issue
	* Use co_external_memory flag in struct vmbus_channel
	   directly instead of adding new flag in the struct
	   hv_device.
---
 drivers/scsi/storvsc_drv.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6977ca8a0658..a3b22cba3421 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1326,7 +1326,8 @@ static void storvsc_on_channel_callback(void *context)
 					continue;
 				}
 				request = (struct storvsc_cmd_request *)scsi_cmd_priv(scmnd);
-				scsi_dma_unmap(scmnd);
+				if (!device->channel->co_external_memory)
+					scsi_dma_unmap(scmnd);
 			}
 
 			storvsc_on_receive(stor_device, packet, request);
@@ -1815,7 +1816,7 @@ static enum scsi_qc_status storvsc_queuecommand(struct Scsi_Host *host,
 		unsigned long offset_in_hvpg = offset_in_hvpage(sgl->offset);
 		unsigned int hvpg_count = HVPFN_UP(offset_in_hvpg + length);
 		struct scatterlist *sg;
-		unsigned long hvpfn, hvpfns_to_add;
+		unsigned long hvpfn, hvpfns_to_add, hvpgoff;
 		int j, i = 0, sg_count;
 
 		payload_sz = (hvpg_count * sizeof(u64) +
@@ -1831,10 +1832,14 @@ static enum scsi_qc_status storvsc_queuecommand(struct Scsi_Host *host,
 		payload->range.len = length;
 		payload->range.offset = offset_in_hvpg;
 
-		sg_count = scsi_dma_map(scmnd);
-		if (sg_count < 0) {
-			ret = SCSI_MLQUEUE_DEVICE_BUSY;
-			goto err_free_payload;
+		if (dev->channel->co_external_memory) {
+			sg_count = scsi_sg_count(scmnd);
+		} else {
+			sg_count = scsi_dma_map(scmnd);
+			if (sg_count < 0) {
+				ret = SCSI_MLQUEUE_DEVICE_BUSY;
+				goto err_free_payload;
+			}
 		}
 
 		for_each_sg(sgl, sg, sg_count, j) {
@@ -1846,9 +1851,16 @@ static enum scsi_qc_status storvsc_queuecommand(struct Scsi_Host *host,
 			 * Such offsets are handled even on other than the first
 			 * sgl entry, provided they are a multiple of PAGE_SIZE.
 			 */
-			hvpfn = HVPFN_DOWN(sg_dma_address(sg));
-			hvpfns_to_add = HVPFN_UP(sg_dma_address(sg) +
-						 sg_dma_len(sg)) - hvpfn;
+			if (dev->channel->co_external_memory) {
+				hvpgoff = HVPFN_DOWN(sg->offset);
+				hvpfn = page_to_hvpfn(sg_page(sg)) + hvpgoff;
+				hvpfns_to_add =	HVPFN_UP(sg->offset + sg->length) -
+							hvpgoff;
+			} else {
+				hvpfn = HVPFN_DOWN(sg_dma_address(sg));
+				hvpfns_to_add = HVPFN_UP(sg_dma_address(sg) +
+							 sg_dma_len(sg)) - hvpfn;
+			}
 
 			/*
 			 * Fill the next portion of the PFN array with
@@ -1870,7 +1882,7 @@ static enum scsi_qc_status storvsc_queuecommand(struct Scsi_Host *host,
 	ret = storvsc_do_io(dev, cmd_request, smp_processor_id());
 	migrate_enable();
 
-	if (ret)
+	if (ret && (!dev->channel->co_external_memory))
 		scsi_dma_unmap(scmnd);
 
 	if (ret == -EAGAIN) {
-- 
2.50.1

Re: [PATCH V2 1/1] x86/VMBus: Confidential VMBus for dynamic DMA transfers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] If storvsc_do_io() returns an error other than -EAGAIN, storvsc_queuecommand() incorrectly returns 0 to the SCSI midlayer, leaking the dynamically allocated payload and causing a command timeout/hang.
--

--- Patch [1]: [PATCH V2 1/1] x86/VMBus: Confidential VMBus for dynamic DMA transfers ---
commit 0fcd974d529c3a7f6e32e98d08ecd19b7a1e7b0a
Author: Tianyu Lan <ltykernel@gmail.com>

x86/VMBus: Confidential VMBus for dynamic DMA transfers
    
This commit modifies the VMBus driver to support dynamic DMA transfers
for Confidential VMs. It introduces the co_external_memory flag to bypass
the swiotlb and DMA API, allowing direct DMA to private encrypted memory.

> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 571ea549152b1..656fe73e0d35e 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c

[ ... ]

> @@ -1870,7 +1882,7 @@ static enum scsi_qc_status storvsc_queuecommand(struct Scsi_Host *host,
>  	ret = storvsc_do_io(dev, cmd_request, smp_processor_id());
>  	migrate_enable();
>  
> -	if (ret)
> +	if (ret && (!dev->channel->co_external_memory))
>  		scsi_dma_unmap(scmnd);
>  
>  	if (ret == -EAGAIN) {

[Severity: High]
This is a pre-existing issue, but if storvsc_do_io() returns an error
other than -EAGAIN, does storvsc_queuecommand() leak the payload buffer
and silently drop the error?

If storvsc_do_io() returns a failure like -ENODEV, the code bypasses the
err_free_payload block and returns 0 to the SCSI midlayer:

storvsc_queuecommand() {
    ...
    ret = storvsc_do_io(dev, cmd_request, smp_processor_id());
    ...
    if (ret == -EAGAIN) {
        ret = SCSI_MLQUEUE_DEVICE_BUSY;
        goto err_free_payload;
    }

    return 0;

err_free_payload:
    if (payload_sz > sizeof(cmd_request->mpb))
        kfree(payload);

    return ret;
}

Since 0 is returned, the midlayer believes the command queued successfully,
but because it was never sent to the host, scsi_done() is never called.
Will this cause the command to linger until a SCSI command timeout occurs
while also permanently leaking the dynamically allocated payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727152711.2045-1-tiala@microsoft.com?part=1