Re: [PATCH net-next v7 2/2] dinghai: add hardware register access and PCI capability scanning

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/
---
dinghai: add hardware register access and PCI capability scanning

Implement PCI configuration space access, BAR mapping, capability
scanning (common/notify/device), and hardware queue register
definitions for DingHai PF device.

> diff --git a/drivers/net/ethernet/zte/dinghai/en_pf.c b/drivers/net/ethernet/zte/dinghai/en_pf.c
> index b492fd2c261b8..fb80b0baca34d 100644
> --- a/drivers/net/ethernet/zte/dinghai/en_pf.c
> +++ b/drivers/net/ethernet/zte/dinghai/en_pf.c

[ ... ]

> @@ -104,6 +105,443 @@ void dh_pf_pci_close(struct dh_core_dev *dev)

[ ... ]

> +void __iomem *zxdh_pf_map_capability(struct dh_core_dev *dh_dev, int off,
> +				     size_t minlen, u32 align,
> +				     u32 start, u32 size,
> +				     size_t *len, resource_size_t *pa,
> +				     u32 *bar_off)
> +{

[ ... ]

> +	if (length > size)
> +		length = size;
> +
> +	if (len)
> +		*len = length;
> +
> +	if (minlen + offset < minlen ||
> +	    minlen + offset > pci_resource_len(pdev, bar)) {

[Severity: High]
Does this code inflate the length returned in *len?

Because length is not capped by pci_resource_len(pdev, bar) - offset before 
being assigned to *len, the returned length can be larger than the actually 
mapped memory (which is capped by pci_iomap_range).

Later in zxdh_pf_map_vq_notify(), this inflated notify_len is used for bounds 
checking, which could allow out-of-bounds MMIO accesses to pass the check.

[ ... ]

> +int zxdh_pf_device_cfg_init(struct dh_core_dev *dh_dev)
> +{
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +	struct pci_dev *pdev = dh_dev->pdev;
> +	int device;
> +
> +	/* Device capability is only mandatory for
> +	 * devices that have device-specific configuration.
> +	 */
> +	device = zxdh_pf_pci_find_capability(pdev, ZXDH_PCI_CAP_DEVICE_CFG,
> +					     IORESOURCE_IO | IORESOURCE_MEM,
> +					     &pf_dev->modern_bars);
> +
> +	/* we don't know how much we should map,
> +	 * but PAGE_SIZE is more than enough for all existing devices.
> +	 */
> +	if (device) {
> +		pf_dev->device = zxdh_pf_map_capability(dh_dev, device, 0,
> +							ZXDH_PF_ALIGN4, 0, PAGE_SIZE,
> +						       &pf_dev->device_len, NULL,
> +						       &pf_dev->dev_cfg_bar_off);
> +		if (!pf_dev->device) {

[Severity: High]
Could pf_dev->device remain NULL if the device capability isn't found?

If the capability is optional and not provided, pf_dev->device will be NULL. 
Also, passing minlen = 0 allows undersized mappings. 

Later, zxdh_get_mac() and zxdh_set_mac() unconditionally dereference 
pf_dev->device, which could trigger a kernel panic due to a NULL pointer 
dereference or out-of-bounds access.

[ ... ]

> +void __iomem *zxdh_pf_map_vq_notify(struct dh_core_dev *dh_dev,
> +				    u16 phy_index, u16 index,
> +				     resource_size_t *pa)
> +{
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +	u16 off;
> +
> +	off = zxdh_pf_get_queue_notify_off(dh_dev, phy_index, index);
> +
> +	if (pf_dev->notify_base) {

[ ... ]

> +	} else {
> +		return zxdh_pf_map_capability(dh_dev, pf_dev->notify_map_cap, 2, 2,
> +					      off * pf_dev->notify_offset_multiplier,
> +					      2, NULL, pa, NULL);
> +	}
> +}

[Severity: Medium]
Can the offset calculation overflow before being passed?

Since off is u16 and pf_dev->notify_offset_multiplier is u32, 
off * pf_dev->notify_offset_multiplier evaluates as a 32-bit unsigned integer. 

If the product exceeds 4GB, it wraps around before being passed to 
zxdh_pf_map_capability(). A 64-bit cast is needed to prevent mapping incorrect 
device registers.

[ ... ]

> +void zxdh_pf_get_vf_mac(struct dh_core_dev *dh_dev, u8 *mac, int vf_id)
> +{
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +	u32 DEV_MAC_L;
> +	u16 DEV_MAC_H;
> +
> +	if (pf_dev->pf_sriov_cap_base) {
> +		DEV_MAC_L = ioread32(pf_dev->pf_sriov_cap_base +
> +				     (pf_dev->sriov_bar_size) * vf_id +
> +				     pf_dev->dev_cfg_bar_off);
> +		mac[0] = DEV_MAC_L & 0xff;
> +		mac[1] = (DEV_MAC_L >> 8) & 0xff;
> +		mac[2] = (DEV_MAC_L >> 16) & 0xff;
> +		mac[3] = (DEV_MAC_L >> 24) & 0xff;
> +		DEV_MAC_H = ioread16(pf_dev->pf_sriov_cap_base +
> +				      (pf_dev->sriov_bar_size) * vf_id +
> +				      pf_dev->dev_cfg_bar_off +
> +				      ZXDH_DEV_MAC_HIGH_OFFSET);
> +		mac[4] = DEV_MAC_H & 0xff;
> +		mac[5] = (DEV_MAC_H >> 8) & 0xff;
> +	}
> +}

[Severity: High]
Does this code silently return without initializing the buffer?

If pf_dev->pf_sriov_cap_base is NULL, the function exits without modifying the 
output mac buffer or returning an error code. 

The caller might proceed using the uninitialized contents of the array, which 
could expose uninitialized kernel stack bytes.

[ ... ]

> +void zxdh_set_mac(struct dh_core_dev *dh_dev, u8 *mac)
> +{
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +	u32 DEV_MAC_L;
> +	u16 DEV_MAC_H;
> +
> +	DEV_MAC_L = mac[0] | (mac[1] << 8) | (mac[2] << 16) | (mac[3] << 24);
> +	DEV_MAC_H = mac[4] | (mac[5] << 8);
> +	iowrite32(DEV_MAC_L, pf_dev->device);
> +	iowrite16(DEV_MAC_H, pf_dev->device + ZXDH_DEV_MAC_HIGH_OFFSET);
> +}
> +
> +void zxdh_get_mac(struct dh_core_dev *dh_dev, u8 *mac)
> +{
> +	struct zxdh_pf_device *pf_dev = dh_dev->priv;
> +	u32 DEV_MAC_L;
> +	u16 DEV_MAC_H;
> +
> +	DEV_MAC_L = ioread32(pf_dev->device);
> +	mac[0] = DEV_MAC_L & 0xff;
> +	mac[1] = (DEV_MAC_L >> 8) & 0xff;
> +	mac[2] = (DEV_MAC_L >> 16) & 0xff;
> +	mac[3] = (DEV_MAC_L >> 24) & 0xff;
> +	DEV_MAC_H = ioread16(pf_dev->device + ZXDH_DEV_MAC_HIGH_OFFSET);
> +	mac[4] = DEV_MAC_H & 0xff;
> +	mac[5] = (DEV_MAC_H >> 8) & 0xff;
> +}

[Severity: High]
Can these functions trigger a NULL pointer dereference?

In zxdh_pf_device_cfg_init(), pf_dev->device can be left as NULL if the device 
capability is missing, or it could be undersized. 

zxdh_set_mac() and zxdh_get_mac() unconditionally dereference pf_dev->device, 
which could trigger a kernel panic.
-- 
This is an AI-generated review.