Re: [PATCH net-next v2 1/8] eth: mpnic: add scaffolding for Meta Platforms NIC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] eth: mpnic: missing PCI BAR0 length validation leads to out-of-bounds MMIO access
--

commit 716880959e1661559f15c4ddde5849a4394122ac
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: add scaffolding for Meta Platforms NIC

This commit introduces a bare PCI driver for the Meta Platforms Network
Interface Controller (MPNIC). It maps BAR0 and provides basic 64-bit MMIO
accessors with a fallback check to detect removed devices.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic.h b/drivers/net/ethernet/meta/mpnic/mpnic.h
> new file mode 100644
> index 0000000000000..6b8bee93033ab
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic.h
[ ... ]
> +static inline void mpnic_wr64(struct mpnic_dev *mpd, u32 reg, u64 val)
> +{
> +	u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
> +
> +	if (csr)
> +		writeq(val, csr + reg);

[Severity: High]
Does this blind pointer arithmetic risk out-of-bounds MMIO writes?

The generic accessor lacks bounds checking on reg, which could result in
writing past the end of the mapped BAR.

[ ... ]
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_pci.c b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
> new file mode 100644
> index 0000000000000..96393e7812418
> --- /dev/null
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_pci.c
[ ... ]
> +u64 mpnic_rd64(struct mpnic_dev *mpd, u32 reg)
> +{
> +	u32 __iomem *csr = READ_ONCE(mpd->uc_addr0);
> +	u64 value;
> +
> +	if (!csr)
> +		return ~0ULL;
> +
> +	value = readq(csr + reg);
> +
> +	/* If any bits are 0 value should be valid */
> +	if (~value)
> +		return value;
> +
> +	/* All ones can be a valid value, so confirm against a register
> +	 * which never reads that way on a live device.
> +	 */
> +	if (reg != MPNIC_BDQ_SPARE && ~readq(csr + MPNIC_BDQ_SPARE))

[Severity: High]
Can this fallback check cause a kernel panic if a malicious or spoofed PCIe
device advertises a small BAR0?

This unconditionally reads from MPNIC_BDQ_SPARE (index 0x42013e, byte
offset 0x10804F8). If the mapped BAR is smaller than ~17.3MB, reading from
this hardcoded offset will crash the kernel.

> +		return value;
> +
> +	mpnic_mmio_err(mpd, reg);
> +
> +	return ~0ULL;
> +}
[ ... ]
> +static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
[ ... ]
> +	mpd = mpnic_alloc(pdev);
> +	if (!mpd)
> +		return -ENOMEM;
> +
> +	uc_addr0 = pcim_iomap_region(pdev, 0, MPNIC_DRV_NAME);

[Severity: High]
Should we verify the device-provided BAR length before mapping it here?

BAR0 is mapped without checking if pci_resource_len(pdev, 0) is large
enough to encompass the driver's register layout. An attacker plugging in
untrusted hardware could advertise a small BAR0 (e.g., 4KB) and return
all-ones to an initial read. This would force the driver to read from
csr + MPNIC_BDQ_SPARE and crash the system.

> +	if (IS_ERR(uc_addr0)) {
> +		err = PTR_ERR(uc_addr0);
> +		dev_err(&pdev->dev, "Mapping the register file failed: %d\n",
> +			err);
> +		goto err_free_mpd;
> +	}
> +	mpd->uc_addr0 = uc_addr0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-linux-mpnic-v2-0-4badc9b58b9e@gmail.com?part=1