Re: [PATCH net] net: mana: hardening: Validate SHM offset from BAR0 register to prevent crash due to alignment fault

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [High] The bounds checks fail to account for the size of the accessed memory regions, allowing out-of-bounds MMIO reads.
- [High] The alignment check for `sriov_base_off` is insufficient and will trigger the exact arm64 alignment fault the patch attempts to fix.
- [High] Pre-existing incomplete bounds check for the doorbell page offset allows out-of-bounds MMIO accesses.
- [Medium] The bounds check for `sriov_base_off + sriov_shm_off` is vulnerable to integer overflow.
--

commit 41b032a76edad75d2aeb45ec344d5df13479815a
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

net: mana: hardening: Validate SHM offset from BAR0 register to prevent crash due to alignment fault

This commit validates the SHM and SRIOV base offsets read from the hardware
registers in the MANA driver to ensure they are within the BAR0 bounds and
aligned to 4 bytes. This aims to prevent alignment faults during hardware
recovery when registers might contain garbage values.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c

[ ... ]

> @@ -73,10 +74,25 @@ static int mana_gd_init_pf_regs(struct pci_dev *pdev)
>  	gc->phys_db_page_base = gc->bar0_pa + gc->db_page_off;
>  
>  	sriov_base_off = mana_gd_r64(gc, GDMA_SRIOV_REG_CFG_BASE_OFF);
> +	if (sriov_base_off >= gc->bar0_size ||
> +	    !IS_ALIGNED(sriov_base_off, sizeof(u32))) {

Does this alignment check ensure readq() won't fault? The code immediately
uses this offset for mana_gd_r64() which performs an 8-byte read. Since
GDMA_PF_REG_SHM_OFF is 0x70 (a multiple of 8), if sriov_base_off is only
4-byte aligned (e.g., 0x4), wouldn't the readq() address be unaligned and
trigger the exact same arm64 alignment fault this patch aims to fix?

Also, is it possible for the subsequent mana_gd_r64() read to access memory
outside the BAR? This checks if sriov_base_off is strictly less than
gc->bar0_size, but mana_gd_r64() reads 8 bytes at
sriov_base_off + GDMA_PF_REG_SHM_OFF. If sriov_base_off is valid but near
the end of the BAR, will this result in an out-of-bounds MMIO read?

> +		dev_err(gc->dev,
> +			"SRIOV base offset 0x%llx out of range or unaligned (BAR0 size 0x%llx)\n",
> +			sriov_base_off, (u64)gc->bar0_size);
> +		return -EPROTO;
> +	}
>  
>  	sriov_base_va = gc->bar0_va + sriov_base_off;
> -	gc->shm_base = sriov_base_va +
> -			mana_gd_r64(gc, sriov_base_off + GDMA_PF_REG_SHM_OFF);
> +
> +	sriov_shm_off = mana_gd_r64(gc, sriov_base_off + GDMA_PF_REG_SHM_OFF);
> +	if (sriov_base_off + sriov_shm_off >= gc->bar0_size ||

Can sriov_base_off + sriov_shm_off integer overflow?

If sriov_shm_off is a large garbage value from the hardware, the sum could
wrap around to a small value, bypassing this check while silently aliasing
another hardware register. Should this be written as
sriov_shm_off >= gc->bar0_size - sriov_base_off?

> +	    !IS_ALIGNED(sriov_shm_off, sizeof(u32))) {

[ ... ]

> @@ -111,7 +128,16 @@ static int mana_gd_init_vf_regs(struct pci_dev *pdev)
>  	gc->db_page_base = gc->bar0_va + gc->db_page_off;
>  	gc->phys_db_page_base = gc->bar0_pa + gc->db_page_off;

This isn't a bug introduced by this patch, but earlier in this function
there is a check for the doorbell offset:

	if (gc->db_page_off >= gc->bar0_size) {
		...

Does this check properly account for the size of the doorbell page? If
db_page_off is valid but less than gc->db_page_size from the end of the BAR,
will subsequent doorbell accesses fall outside the mapped BAR0 memory region?

> -	gc->shm_base = gc->bar0_va + mana_gd_r64(gc, GDMA_REG_SHM_OFFSET);
> +	shm_off = mana_gd_r64(gc, GDMA_REG_SHM_OFFSET);
> +	if (shm_off >= gc->bar0_size ||

Does this bounds check ensure that subsequent reads don't go out of bounds?

shm_off is used to set gc->shm_base, and the commit message mentions
mana_smc_poll_register() accesses gc->shm_base up to offset 0x1c (28 bytes).
If shm_off passes this check but is near the end of the BAR
(e.g., gc->bar0_size - 4), will the driver perform an out-of-bounds MMIO
read?

> +	    !IS_ALIGNED(shm_off, sizeof(u32))) {
> +		dev_err(gc->dev,
> +			"SHM offset 0x%llx out of range or unaligned (BAR0 size 0x%llx)\n",
> +			shm_off, (u64)gc->bar0_size);
> +		return -EPROTO;
> +	}
> +
> +	gc->shm_base = gc->bar0_va + shm_off;
>  
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/aepF3NwyANeklkfD@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net?part=1