Re: [PATCH v2 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:
- [High] Unkillable sleep in device removal path if userspace keeps the memory mapped.
- [High] TOCTOU race in leapraid_fw_mmap allowing use-after-free of fw_log_buffer.
--

--- Patch [1]: [PATCH v2 1/2] scsi: leapraid: balance host references for firmware log VMAs ---
commit f0e08e76b84e5bcaddf6147e197d89f0cec2a4e9
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.

The open callback is also invoked when a VMA is duplicated or split,
but it only increments mmap_refcnt.  Since every corresponding close
callback drops a host reference, cloning the mapping can release the
host while another VMA still refers to the adapter.

Take a host device reference for every VMA open and release the lookup
reference once the initial mapping has acquired its own reference.  Use
get_device() because a VMA can be cloned after the host enters SHOST_DEL;
an existing VMA still pins the host at that point and open cannot fail.

> 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 create an unkillable sleep in the
device removal path?

If a user maps the firmware log buffer via mmap and never unmaps it, the
removal thread executing leapraid_fw_log_exit() in leapraid_func.c will block
indefinitely waiting for the reference count to drop to zero:

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

Since there is no mechanism to forcibly revoke the mapping, could this allow
userspace to block device removal indefinitely?

>  }
>  
> @@ -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: High]
This isn't a bug introduced by this patch, but is there a race condition here
that allows a use-after-free of the firmware log buffer?

The buffer is mapped to user space via dma_mmap_coherent() earlier in this
function before mmap_refcnt is incremented here in leapraid_fw_mmap_open().

If a concurrent device removal preempts this thread right before
leapraid_fw_mmap_open() is called, leapraid_fw_log_exit() will see
mmap_refcnt == 0 and free the buffer via dma_free_coherent().

When the mmap thread resumes, will it have successfully mapped freed kernel
DMA memory into user space?

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

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