Re: [PATCH net-next 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] mpnic: Missing PCI BAR size validation causing OOB MMIO access
--

commit 04a97e4905a47ed1a317515fbb919eef32a497b2
Author: Daniel Zahka <daniel.zahka@gmail.com>

eth: mpnic: add scaffolding for Meta Platforms NIC

This commit adds the initial bare PCI driver scaffolding for the Meta
Platforms Network Interface Controller. It introduces basic MMIO read/write
operations and PCI device registration.

> 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]
Could this unconditionally read at a large offset (~16.5MB) regardless of the
actual mapped BAR size? If a device (or an attacker via hotplug/virtualization)
presents a smaller BAR, wouldn't this cause an out-of-bounds MMIO access
and potentially a kernel page fault?

> +		return value;
> +
> +	mpnic_mmio_err(mpd, reg);
> +
> +	return ~0ULL;
> +}

[ ... ]

> +static int mpnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{
> +	void __iomem *uc_addr0;
> +	struct mpnic_dev *mpd;
> +	int err;

[ ... ]

> +	mpd = mpnic_alloc(pdev);
> +	if (!mpd)
> +		return -ENOMEM;
> +
> +	uc_addr0 = pcim_iomap_region(pdev, 0, MPNIC_DRV_NAME);

[Severity: High]
Does this code map the PCI BAR without first validating that the resource
length is large enough to cover the expected register offsets? Should this
validate pci_resource_len(pdev, 0) before mapping to ensure that accesses
like MPNIC_BDQ_SPARE remain within bounds?

> +	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/20260922-linux-mpnic-v1-0-236844f53072@gmail.com?part=1