Re: [PATCH net, v3] net: mana: Fix crash from unvalidated SHM offset read from BAR0 during FLR

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The hardware register `db_page_off` lacks alignment and sufficient upper-bound checks, leaving it vulnerable to the exact same FLR garbage-value kernel crash the patch aims to fix.
--

commit 51ede83a20aa96b2dface8ca0c0ba00d88085dc6
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>

net: mana: Fix crash from unvalidated SHM offset read from BAR0 during FLR

This commit adds validation for shared memory and SR-IOV base offsets read
from hardware registers during Function Level Reset recovery. It ensures the
values are within bar0 bounds and properly aligned to prevent arm64 alignment
faults or out-of-bounds mmio accesses when reading garbage values.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 098fbda0d128a..d8e816882f02c 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -73,10 +74,28 @@ static int mana_gd_init_pf_regs(struct pci_dev *pdev)
>  	gc->phys_db_page_base = gc->bar0_pa + gc->db_page_off;

Should the doorbell offset (db_page_off) also receive the same hardening
against garbage values?

Looking at the existing code just above this hunk, db_page_off is read from
hardware and only validated against the strict upper bound of gc->bar0_size:

        gc->db_page_off = mana_gd_r64(gc, GDMA_PF_REG_DB_PAGE_OFF);
        if (gc->db_page_off >= gc->bar0_size) {
        ...

If the hardware temporarily returns an unaligned garbage value, or a value
that leaves not enough space for the doorbell page (e.g., gc->bar0_size - 2),
this check will pass. Subsequent doorbell writes to gc->db_page_base + offset
via writeq() could trigger an unaligned mmio access or out-of-bounds write.

On arm64, unaligned accesses to Device memory strictly trigger a kernel panic
(alignment fault), which is the exact Function Level Reset recovery issue this
patch aims to fix for the shared memory offsets.

>  	sriov_base_off = mana_gd_r64(gc, GDMA_SRIOV_REG_CFG_BASE_OFF);
> +	if (sriov_base_off >= gc->bar0_size ||
> +	    gc->bar0_size - sriov_base_off <
> +		GDMA_PF_REG_SHM_OFF + sizeof(u64) ||
> +	    !IS_ALIGNED(sriov_base_off, sizeof(u64))) {
> +		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;
> +	}

[ ... ]

> @@ -111,7 +131,17 @@ 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;

A similar incomplete check exists for the VF doorbell offset read from
GDMA_REG_DB_PAGE_OFFSET just above this hunk. Does this offset also need
to be aligned and leave sufficient space in bar0?

> -	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 ||
> +	    gc->bar0_size - shm_off < SMC_APERTURE_SIZE ||
> +	    !IS_ALIGNED(shm_off, sizeof(u32))) {

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