Re: [PATCH] scsi: mpt3sas: fix OOB SGE write at the request frame boundary

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: mpt3sas: incomplete bounds check leaves OOB write unpatched
--

commit c6cf70bbba6355fabaab51bc07707d150970dad8
Author: Weibin Liu <liuwb@xiaopeng.com>

scsi: mpt3sas: fix OOB SGE write at the request frame boundary

_ctl_do_mpt_command() bounds the user-supplied data_sge_offset with
strict inequalities. When data_sge_offset * 4 is exactly ioc->request_sz,
both comparisons pass, allowing an OOB write. This commit changes the
comparison to >= to reject offsets that point exactly at or past the
end of the request frame.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_ctl.c b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> index 8bb947004885b..d8d3ee11ed520 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_ctl.c
> @@ -739,7 +739,7 @@ _ctl_do_mpt_command(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command karg,
>  	}
>  
>  	/* Check for overflow and wraparound */
> -	if (karg.data_sge_offset * 4 > ioc->request_sz ||
> +	if (karg.data_sge_offset * 4 >= ioc->request_sz ||
>  	    karg.data_sge_offset > (UINT_MAX / 4)) {

[Severity: High]
Does this bounds check leave an out-of-bounds write unpatched?

If a user issues an MPT3COMMAND ioctl with karg.data_sge_offset set such
that karg.data_sge_offset * 4 is slightly less than ioc->request_sz
(e.g., ioc->request_sz - 4), the updated validation still passes because
ioc->request_sz - 4 is not >= ioc->request_sz.

The SGE array pointer psge would then be set to point 4 bytes before the
end of the request frame. The SGE builder functions (ioc->build_sg, such
as _base_build_sg() or _base_build_zero_len_sge()) write a minimum of 8
to 32 bytes of scatter-gather elements to psge without further bounds
checking:

drivers/scsi/mpt3sas/mpt3sas_base.c:_base_build_sg() {
...
	if (!data_out_sz && !data_in_sz) {
		_base_build_zero_len_sge(ioc, psge);
		return;
	}
...
}

Since only 4 bytes remain in the frame, the SGE write overflows the request
buffer by at least 4 bytes. Because the request buffers are allocated
contiguously in a DMA pool array, does this overflow corrupt the adjacent
request frame?

>  		ret = -EINVAL;
>  		goto out;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-mpt3sas-sge-offset-v1-1-9b91c265f2c1@xiaopeng.com?part=1