Re: [PATCH v3 1/3] scsi: arcmsr: bound the firmware-supplied message length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: arcmsr: TOCTOU double fetch causes circular buffer corruption
--

commit c1b87442346f4c23552402e5fce0db15b1252056
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: arcmsr: bound the firmware-supplied message length

This commit addresses an out-of-bounds write where an unbounded data_len
from the MMIO register could walk past the 128-byte staging buffer. It
clamps the length to the size of the window (124 bytes) in both reader
functions.

> diff --git a/drivers/scsi/arcmsr/arcmsr_hba.c b/drivers/scsi/arcmsr/arcmsr_hba.c
> index 8aa948f06cacf..255f1d898079f 100644
> --- a/drivers/scsi/arcmsr/arcmsr_hba.c
> +++ b/drivers/scsi/arcmsr/arcmsr_hba.c
> @@ -2277,6 +2277,8 @@ arcmsr_Read_iop_rqbuffer_in_DWORD(struct AdapterControlBlock *acb,
>  
>  	iop_data = (uint32_t __iomem *)prbuffer->data;
>  	iop_len = readl(&prbuffer->data_len);
> +	if (iop_len > sizeof(prbuffer->data))
> +		iop_len = sizeof(prbuffer->data);

[Severity: Medium]
The commit message notes that the adapter can change data_len between the two
reads. While clamping iop_len to sizeof(prbuffer->data) prevents the staging
buffer overflow, does this still leave a TOCTOU (time-of-check to time-of-use)
vulnerability that can corrupt the circular buffer?

arcmsr_iop2drv_data_wrote_handle() validates the first fetch against
buf_empty_len:

drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_iop2drv_data_wrote_handle() {
	...
	if (buf_empty_len >= readl(&prbuffer->data_len)) {
		if (arcmsr_Read_iop_rqbuffer_data(acb, prbuffer) == 0)
			...
}

If buf_empty_len is small (e.g., 10 bytes), the first fetch could be 10, passing
the check. If the second fetch here sees an increased data_len from the MMIO
register (e.g., 124 bytes), the while loop below will write more bytes than
buf_empty_len.

Could this cause rqbuf_putIndex to overtake rqbuf_getIndex and corrupt the
driver's internal ring buffer state?

>  	if (iop_len > 0) {
>  		buf1 = kmalloc(128, GFP_ATOMIC);
>  		buf2 = (uint32_t *)buf1;

[ ... ]

> @@ -2319,6 +2321,8 @@ arcmsr_Read_iop_rqbuffer_data(struct AdapterControlBlock *acb,
>  		return arcmsr_Read_iop_rqbuffer_in_DWORD(acb, prbuffer);
>  	iop_data = (uint8_t __iomem *)prbuffer->data;
>  	iop_len = readl(&prbuffer->data_len);
> +	if (iop_len > sizeof(prbuffer->data))
> +		iop_len = sizeof(prbuffer->data);

[Severity: Medium]
This second fetch in the byte-wise reader shares the same vulnerability when
called from arcmsr_iop2drv_data_wrote_handle().

Would passing the validated length down from the caller prevent the hardware
from bypassing the buf_empty_len check entirely?

>  	while (iop_len > 0) {
>  		pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
>  		*pQbuffer = readb(iop_data);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791803953.git.yhlee@isslab.korea.ac.kr?part=1