Re: [PATCH] scsi: advansys: Fix invalid comparison in AdvBuildCarrierFreelist

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

Pre-existing issues:
- [High] Boundary checks in `adv_get_carrier()` and `adv_get_reqp()` contain off-by-one errors by using strict inequality (`>`), allowing out-of-bounds pointer dereferences if the hardware provides specific boundary offsets.
- [High] Missing `dma_rmb()` and `dma_wmb()` barriers in the hardware DMA ring (carrier) management lead to race conditions with the device.
--

--- Patch [1]: [PATCH] scsi: advansys: Fix invalid comparison in AdvBuildCarrierFreelist ---
commit 1fec77d68ab535d94e22469fc8d09c10da06eab9
Author: Roman Demidov <[email protected]>

scsi: advansys: Fix invalid comparison in AdvBuildCarrierFreelist

The loop index i takes values from 0 to carr_num - 1 therefore
the comparison i == carr_num is always false. The assignment
next_offset = ~0 must occur for the last element of the list,
its index is carr_num - 1.

> diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
> index 5cdbf2bdb13df..95a227420b283 100644
> --- a/drivers/scsi/advansys.c
> +++ b/drivers/scsi/advansys.c
> @@ -4169,7 +4169,7 @@ static void AdvBuildCarrierFreelist(struct adv_dvc_var *adv_dvc)
>  		adv_dvc->carrier[i].carr_va = cpu_to_le32(carr_offset);
>  		adv_dvc->carrier[i].areq_vpa = 0;
>  		next_offset = carr_offset + sizeof(ADV_CARR_T);
> -		if (i == carr_num)
> +		if (i == carr_num - 1)
>  			next_offset = ~0;
>  		adv_dvc->carrier[i].next_vpa = cpu_to_le32(next_offset);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
out-of-bounds dereference in adv_get_carrier(). The driver receives offsets
from the device and validates them:

drivers/scsi/advansys.c:adv_get_carrier()
    ...
    BUG_ON(offset > ADV_CARRIER_BUFSIZE);
    ...

Could this allow an offset exactly equal to ADV_CARRIER_BUFSIZE to pass
the check, resulting in an index pointing one element past the end of the
carrier array? Should this use greater-than-or-equal instead?

The same pattern exists in adv_get_reqp():

drivers/scsi/advansys.c:adv_get_reqp()
    ...
    BUG_ON(offset > adv_dvc->max_host_qng);
    ...

Can an offset equal to max_host_qng bypass the safeguard and cause memory
corruption if malicious hardware provides this exact value?

[Severity: High]
This is a pre-existing issue, but are there missing DMA barriers in the
hardware DMA ring management?

In the submission path:

drivers/scsi/advansys.c:AdvExeScsiQueue()
    ...
    asc_dvc->icq_sp->areq_vpa = scsiq->scsiq_rptr;
    asc_dvc->icq_sp->next_vpa = new_carrp->carr_pa;
    ...

Without a dma_wmb() between writing the payload (areq_vpa) and the valid
flag (next_vpa), could weakly ordered CPUs reorder the stores, causing the
hardware to observe the new next_vpa while still reading a stale areq_vpa?

Similarly, in the completion path:

drivers/scsi/advansys.c:AdvISR()
    ...
    while (((irq_next_vpa = le32_to_cpu(asc_dvc->irq_sp->next_vpa)) & ADV_RQ_DONE) != 0) {
        u32 pa_offset = le32_to_cpu(asc_dvc->irq_sp->areq_vpa);
    ...

Without a dma_rmb() after checking the done flag (next_vpa), could weakly
ordered CPUs speculatively read the payload field (areq_vpa) before the
device has written it, leading to the processing of stale state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1