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

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 <[email protected]>

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/[email protected]?part=1