Re: [PATCH v2 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 on prbuffer->data_len allows ring buffer corruption
--

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

scsi: arcmsr: bound the firmware-supplied message length

This commit bounds the firmware-supplied message length to prevent out-of-bounds
writes. It clamps the length of inbound messages to the 128-byte window size
during reads from the adapter. This fixes an issue where a malicious or
misbehaving firmware could specify an oversized data length and overrun the
allocated buffer.

>     The adapter hands the driver an inbound message through a fixed
>     128-byte window:
>     
>             struct QBUFFER {
>                     uint32_t        data_len;
>                     uint8_t         data[124];
>             };
>     
>     arcmsr_Read_iop_rqbuffer_in_DWORD() takes data_len from that window and
>     uses it as the copy length without comparing it against the window:

[ ... ]

>     Two of the three callers place no bound on the value at all:
>     arcmsr_iop_message_xfer() for ARCMSR_MESSAGE_READ_RQBUFFER and
>     arcmsr_sysfs_iop_message_read() both call straight into the reader on
>     ACB_F_IOPDATA_OVERFLOW.  The third, arcmsr_iop2drv_data_wrote_handle(),
>     compares data_len against the free space in acb->rqbuffer, which is up
>     to ARCMSR_MAX_QBUFFER - 1 and so still far larger than the 128-byte
>     staging buffer; it also reads data_len separately from the reader, so
>     the adapter can change it between the two reads.
>     
>     Clamp the length to the size of the window in both readers.  A
>     conforming adapter never reports more than sizeof(prbuffer->data), so
>     this does not drop any data it sends.

[ ... ]

> 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]
This problem wasn't introduced by this patch, but as noted in the commit
message, the adapter can change data_len between reads. Can this still lead
to ring buffer corruption?

In drivers/scsi/arcmsr/arcmsr_hba.c:arcmsr_iop2drv_data_wrote_handle(),
the first fetch checks available space:

	if (buf_empty_len >= readl(&prbuffer->data_len)) {
		if (arcmsr_Read_iop_rqbuffer_data(acb, prbuffer) == 0)

Then here in arcmsr_Read_iop_rqbuffer_in_DWORD() (and similarly in
arcmsr_Read_iop_rqbuffer_data()), data_len is fetched again directly from
MMIO.

If the firmware changes the value from a small size (e.g., 4) to 124 between
the two reads, it bypasses the buf_empty_len check. Even though this patch
clamps the length to sizeof(prbuffer->data), it can still write up to 124 bytes
into a ring buffer that lacks sufficient space.

This advances acb->rqbuf_putIndex past acb->rqbuf_getIndex. Because the ring
buffer calculation relies on the difference between these pointers, does this
wrap cause previously unread messages to be silently dropped and corrupt the
message stream to userspace?

Could the length be read once in arcmsr_iop2drv_data_wrote_handle() and passed
down to the read functions to avoid the double fetch?

>  	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);
>  	while (iop_len > 0) {
>  		pQbuffer = &acb->rqbuffer[acb->rqbuf_putIndex];
>  		*pQbuffer = readb(iop_data);

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