Re: [PATCH] scsi: megaraid: clear ioctl DMA buffers before use

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Sleeping in Atomic Context during DMA buffer allocation
- [High] DMA Buffer Overflow due to bypassed bounds check on read-only passthrough commands
- [High] Invalid Free and potential NULL pointer dereference on allocation failure path
--

commit ddb879c51888ddd8509956e8e975f6048a188af1
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

scsi: megaraid: clear ioctl DMA buffers before use

The MIMD ioctl path reuses DMA buffers from per-adapter pools, or
allocates a fresh pool buffer when the shared buffers are busy. Read
commands copy the requested user-visible length back after firmware
completion, but firmware is not guaranteed to overwrite every byte in the
bounce buffer.

Clear the attached DMA buffer before issuing the command so short device
writes cannot return stale data from a previous ioctl or allocation. This
covers both regular DCMDs and passthrough commands after their requested
lengths have been validated.

> diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
> index a70692779a16c..6e0db096215c7 100644
> --- a/drivers/scsi/megaraid/megaraid_mm.c
> +++ b/drivers/scsi/megaraid/megaraid_mm.c

[ ... ]

> @@ -577,6 +578,8 @@ mraid_mm_attach_buf(mraid_mmadp_t *adp, uioc_t *kioc, int xferlen)
>  	if (!kioc->buf_vaddr)
>  		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does the allocation right before this check
sleep while holding a spinlock?

drivers/scsi/megaraid/megaraid_mm.c:mraid_mm_attach_buf() {
...
	spin_lock_irqsave(&pool->lock, flags);

	kioc->pool_index	= right_pool;
	kioc->free_buf		= 1;
	kioc->buf_vaddr 	= pci_pool_alloc(pool->handle, GFP_KERNEL,
							&kioc->buf_paddr);
	spin_unlock_irqrestore(&pool->lock, flags);

	if (!kioc->buf_vaddr)
		return -ENOMEM;
...
}

Calling pci_pool_alloc() with GFP_KERNEL can sleep, which could cause a
scheduling while atomic panic since pool->lock is held with interrupts
disabled.

[Severity: High]
This isn't a bug introduced by this patch, but can the failure path cause a
NULL pointer dereference?

If pci_pool_alloc() fails, it returns -ENOMEM with kioc->free_buf already set
to 1. When the caller propagates the error and invokes
mraid_mm_dealloc_kioc():

drivers/scsi/megaraid/megaraid_mm.c:mraid_mm_dealloc_kioc() {
...
		if (kioc->free_buf == 1)
			pci_pool_free(pool->handle, kioc->buf_vaddr, 
							kioc->buf_paddr);
...
}

This would attempt to free the memory by calling pci_pool_free() with a NULL
virtual address.

> +	memset(kioc->buf_vaddr, 0, xferlen);
> +
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but could the bounds check in mimd_to_kioc()
leave device-to-memory transfers completely unvalidated?

drivers/scsi/megaraid/megaraid_mm.c:mimd_to_kioc() {
...
	pthru32->dataxferaddr	= kioc->buf_paddr;
	if (kioc->data_dir & UIOC_WR) {
		if (pthru32->dataxferlen > kioc->xferlen)
			return -EINVAL;
		if (copy_from_user(kioc->buf_vaddr, kioc->user_data,
						pthru32->dataxferlen)) {
			return (-EFAULT);
		}
	}
...
}

The bounds check for dataxferlen is nested inside
if (kioc->data_dir & UIOC_WR). If the command is a read-only transfer
(UIOC_WR is unset), this check is skipped. A user could issue an ioctl
with a small xferlen but a huge dataxferlen, instructing the firmware
to write past the allocated bounce buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625090101.4761-1-alhouseenyousef@gmail.com?part=1