[PATCH] scsi: megaraid: cap passthrough copyout length

[PATCH] scsi: megaraid: cap passthrough copyout length

[Yousef Alhouseen]

MIMD passthrough commands store the DMA transfer length in dataxferlen.

The common ioctl path copied xferlen bytes back to userspace instead.

For read commands, a larger xferlen can expose stale DMA buffer bytes.

Those bytes are beyond the data returned by the controller.

Validate dataxferlen for both directions and copy back only that length.

Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
 drivers/scsi/megaraid/megaraid_mm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
index e57266590..75f6b7198 100644
--- a/drivers/scsi/megaraid/megaraid_mm.c
+++ b/drivers/scsi/megaraid/megaraid_mm.c
@@ -480,12 +480,14 @@ mimd_to_kioc(mimd_t __user *umimd, mraid_mmadp_t *adp, uioc_t *kioc)
 		return (-EFAULT);
 	}
 
+	if (pthru32->dataxferlen > kioc->xferlen)
+		return -EINVAL;
+	kioc->user_data_len = pthru32->dataxferlen;
+
 	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)) {
+							pthru32->dataxferlen)) {
 			return (-EFAULT);
 		}
 	}
-- 
2.54.0

Re: [PATCH] scsi: megaraid: cap passthrough copyout length

[sashiko-bot]

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

Pre-existing issues:
- [Critical] DMA buffer overflow leading to physical memory corruption for non-passthrough commands.
- [High] Stale DMA buffer information leak via short reads for both passthrough and non-passthrough commands.
--

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

scsi: megaraid: cap passthrough copyout length

MIMD passthrough commands store the DMA transfer length in dataxferlen.
The common ioctl path copied xferlen bytes back to userspace instead.
For read commands, a larger xferlen can expose stale DMA buffer bytes.

> diff --git a/drivers/scsi/megaraid/megaraid_mm.c b/drivers/scsi/megaraid/megaraid_mm.c
> index a70692779a16c..1e49b437e8e5f 100644
> --- a/drivers/scsi/megaraid/megaraid_mm.c
> +++ b/drivers/scsi/megaraid/megaraid_mm.c
> @@ -484,12 +484,14 @@ mimd_to_kioc(mimd_t __user *umimd, mraid_mmadp_t *adp, uioc_t *kioc)
>  		return (-EFAULT);
>  	}
>  
> +	if (pthru32->dataxferlen > kioc->xferlen)
> +		return -EINVAL;
> +	kioc->user_data_len = pthru32->dataxferlen;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this still leave an
information leak if the hardware performs a short read?

Since pthru32->dataxferlen is fully user-controlled and does not reflect
the actual byte count transferred by the firmware, a user can request a large
transfer size but send a command that returns fewer bytes.

Because mraid_mm_attach_buf() allocates the DMA buffer without zeroing it,
won't kioc_to_mimd() then copy uninitialized kernel pool memory back to
userspace based on this inflated kioc->user_data_len?

[Severity: Critical]
This is a pre-existing issue, but while this patch fixes the bounds check
for MBOXCMD_PASSTHRU commands, what about regular DCMD commands?

Looking earlier in mimd_to_kioc(), for non-passthrough commands, the driver
copies the raw 14-byte mailbox from userspace:

	memcpy(mbox, mimd.mbox, 14);

	if (mbox->cmd != MBOXCMD_PASSTHRU) {
		mbox->xferaddr	= (uint32_t)kioc->buf_paddr;
		...
		return 0;
	}

If a user allocates a small DMA pool buffer via a small mimd.outlen but
embeds a huge transfer size in the raw mailbox payload, there doesn't
appear to be any validation against kioc->xferlen.

When mbox_post_cmd() issues this to the firmware, could it cause an
out-of-bounds DMA transfer into adjacent physical memory?

>  	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)) {
> +							pthru32->dataxferlen)) {
>  			return (-EFAULT);
>  		}
>  	}

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