Re: [PATCH v3 1/2] scsi: leapraid: balance host references for firmware log VMAs

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

Pre-existing issues:
- [Critical] TOCTOU race condition in `leapraid_fw_mmap` leading to Use-After-Free of DMA buffer.
- [High] Device removal path hangs uninterruptibly waiting for userspace to close VMAs.
--

--- Patch [1]: [PATCH v3 1/2] scsi: leapraid: balance host references for firmware log VMAs ---
commit 727a052aa98a29cd5e6da31ed634d370a79651cc
Author: Linmao Li <[email protected]>

scsi: leapraid: balance host references for firmware log VMAs

leapraid_fw_mmap() keeps the Scsi_Host reference obtained while
looking up the adapter for the lifetime of the initial VMA.  The VMA
close callback drops that reference.

> diff --git a/drivers/scsi/leapraid/leapraid_app.c b/drivers/scsi/leapraid/leapraid_app.c
> index 742f19c07fcbb..841027ce2501c 100644
> --- a/drivers/scsi/leapraid/leapraid_app.c
> +++ b/drivers/scsi/leapraid/leapraid_app.c
> @@ -697,6 +697,7 @@ static void leapraid_fw_mmap_open(struct vm_area_struct *vma)
>  	if (!adapter)
>  		return;
>  
> +	get_device(&adapter->shost->shost_gendev);
>  	atomic_inc(&adapter->fw_log_desc.mmap_refcnt);

[Severity: High]
This is a pre-existing issue, but does this reference counting lead to
an uninterruptible hang during device removal?

Looking at the teardown path in leapraid_remove_ctrl() ->
leapraid_fw_log_exit(), the kernel waits for this reference count
to hit zero:

    wait_event(adapter->fw_log_desc.mmap_waitq,
               !atomic_read(&adapter->fw_log_desc.mmap_refcnt));

Since there is no timeout and no alternative wake condition, couldn't any
userspace process that keeps the VMA open indefinitely cause the kernel thread
running the removal to hang, creating a denial of service?

>  }

[ ... ]

> @@ -767,7 +768,6 @@ static int leapraid_fw_mmap(struct file *filp, struct vm_area_struct *vma)
>  	vma->vm_private_data = adapter;
>  	vma->vm_ops = &leapraid_fw_mmap_vm_ops;
>  	leapraid_fw_mmap_open(vma);

[Severity: Critical]
This is a pre-existing issue, but does this sequence create a race condition
leading to a use-after-free of the DMA buffer?

Just prior to this snippet, dma_mmap_coherent() is called locklessly and
before mmap_refcnt is incremented here via leapraid_fw_mmap_open().

If a concurrent device removal starts right before dma_mmap_coherent(), the
removal thread would see mmap_refcnt == 0. It would then proceed to free
fw_log_buffer via dma_free_coherent() in leapraid_fw_log_exit().

Could the mmap thread then resume and map the now-freed DMA memory into
user space?

> -	adapter = NULL;
>  
>  	rc = 0;
>  out_put:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1