Re: [PATCH net, v2] 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 4 potential issue(s) to consider:
- [High] Incomplete fix: Missing bounds and alignment validation for the doorbell page offset (`db_page_off`), leaving the driver vulnerable to out-of-bounds MMIO accesses and arbitrary physical memory mapping.
- [High] Missing minimum BAR0 size validation for fixed-offset configuration registers.
- [Low] Duplicate macro definitions for SMC_APERTURE_BITS and related constants.
- [Low] Dangling sentence fragment in the commit message.
--

commit a719bcc10e344c9685c78dde6408c1022c5a812e
Author: Dipayaan Roy <dipayanroy@linux.microsoft.com>
net: mana: Fix crash from unvalidated SHM offset read from BAR0 during FLR

This commit validates the SHM offset read from BAR0 to prevent out-of-bounds
and unaligned accesses. It computes safely by subtracting from bar0_size
instead of adding.

> without validating the offset read from hardware. If the register
> returns a garbage value that is neither within bar 0 bounds nor aligned
> to the 4-byte granularity, thus causing the alignment fault.

This isn't a bug, but there appears to be a dangling sentence fragment left
over in the commit message.

> diff --git a/drivers/net/ethernet/microsoft/mana/gdma_main.c b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> index 098fbda0d128..d8e816882f02 100644
> --- a/drivers/net/ethernet/microsoft/mana/gdma_main.c
> +++ b/drivers/net/ethernet/microsoft/mana/gdma_main.c
> @@ -43,8 +43,9 @@ static u64 mana_gd_r64(struct gdma_context *g, u64 offset)
>  static int mana_gd_init_pf_regs(struct pci_dev *pdev)
>  {
>  	struct gdma_context *gc = pci_get_drvdata(pdev);
> -	void __iomem *sriov_base_va;
> +	u64 remaining_barsize;
>  	u64 sriov_base_off;
> +	u64 sriov_shm_off;
>  
>  	gc->db_page_size = mana_gd_r32(gc, GDMA_PF_REG_DB_PAGE_SIZE) & 0xFFFF;

Is gc->bar0_size guaranteed to be large enough before this initial read?

If a device exposes an unusually small BAR0, could this read from the fixed
GDMA_PF_REG_DB_PAGE_SIZE offset result in an out-of-bounds MMIO access
before the dynamic offset validations are reached?

[ ... ]

> @@ -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;
>  
>  	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;
> +	}

While this cleanly fixes the bounds and alignment checks for sriov_base_off,
should similar checks be applied to db_page_off which is read just above
this section?

In mana_gd_init_pf_regs():
        gc->db_page_off = mana_gd_r64(gc, GDMA_PF_REG_DB_PAGE_OFF);

        if (gc->db_page_off >= gc->bar0_size) {
                dev_err(gc->dev, ...);
                return -EPROTO;
        }

Could this leave a regression with out-of-bounds MMIO accesses or alignment
faults if gc->bar0_size - gc->db_page_off is smaller than the required
doorbell page size?

> diff --git a/include/net/mana/shm_channel.h b/include/net/mana/shm_channel.h
> index 5199b41497ff..dbabcfb95daf 100644
> --- a/include/net/mana/shm_channel.h
> +++ b/include/net/mana/shm_channel.h
> @@ -4,6 +4,12 @@
>  #ifndef _SHM_CHANNEL_H
>  #define _SHM_CHANNEL_H
>  
> +#define SMC_APERTURE_BITS 256
> +#define SMC_BASIC_UNIT (sizeof(u32))
> +#define SMC_APERTURE_DWORDS (SMC_APERTURE_BITS / (SMC_BASIC_UNIT * 8))
> +#define SMC_LAST_DWORD (SMC_APERTURE_DWORDS - 1)
> +#define SMC_APERTURE_SIZE  (SMC_APERTURE_BITS / 8)
> +
>  struct shm_channel {
>  	struct device *dev;
>  	void __iomem *base;

Does exporting these macros to the header file result in duplicate
definitions? 

It looks like the original definitions might still be present in
drivers/net/ethernet/microsoft/mana/shm_channel.c.

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