Re: [PATCH net-next v4 02/15] libie: add PCI device initialization helpers to libie

[Larysa Zaremba <larysa.zaremba@intel.com>]

Sashiko has 1 concern about this patch, and I think it is not very relevant, as 
this is a 32-bit issue on a 200G+ HW, which AI also mentions. Seems like 
checking offsets under ifdefs would be unneccessarily defensive.

Sashiko says:
> commit 0540f7038f92c56e3d90344d11ab6f07b9f6b975
> Author: Phani R Burra <phani.r.burra@intel.com>
> libie: add PCI device initialization helpers to libie
> This patch introduces libie helpers for PCI init, MMIO mapping bookkeeping
> via a list of libie_pci_mmio_region entries, and a lookup helper to
> translate offsets to virtual addresses. The public map/get APIs accept
> resource_size_t offset and size arguments and are intended to be shared by
> idpf and ixd.
> > diff --git a/drivers/net/ethernet/intel/libie/pci.c b/drivers/net/ethernet/intel/libie/pci.c
> > new file mode 100644
> > index 000000000000..a0d3535d6529
> > --- /dev/null
> > +++ b/drivers/net/ethernet/intel/libie/pci.c
> [ ... ]
> > +bool __libie_pci_map_mmio_region(struct libie_mmio_info *mmio_info,
> > +				 resource_size_t offset,
> > +				 resource_size_t size, int num_args, ...)
> > +{
> > +	struct pci_dev *pdev = mmio_info->pdev;
> > +	struct libie_pci_mmio_region *mr;
> > +	resource_size_t end_offset;
> > +	void __iomem *va;
> > +	int bar_idx = 0;
> > +	va_list args;
> > +
> > +	if (num_args) {
> > +		va_start(args, num_args);
> > +		bar_idx = va_arg(args, int);
> > +		va_end(args);
> > +	}
> > +
> > +	/* pci_iomap_range() would silently map less in such case */
> > +	if (!size || check_add_overflow(offset, size, &end_offset) ||
> > +	    end_offset > pci_resource_len(pdev, bar_idx))
> > +		return false;
> Can this API silently truncate the mapping on 32-bit PAE builds?
> The parameters offset and size are resource_size_t, and the bounds check
> above is performed in resource_size_t arithmetic against
> pci_resource_len(), which is fine.
> However, pci_iomap_range() is declared as:
>   void __iomem *pci_iomap_range(struct pci_dev *dev, int bar,
>                                 unsigned long offset,
>                                 unsigned long maxlen);
> On 32-bit configs with CONFIG_PHYS_ADDR_T_64BIT (for example PAE),
> resource_size_t is 64-bit while unsigned long is 32-bit.
> > +	va = pci_iomap_range(mmio_info->pdev, bar_idx, offset, size);
> If a caller passes an offset or size >= 4 GiB that legitimately fits
> within a 64-bit BAR, the libie-level bounds check passes but the values
> are silently truncated when handed to pci_iomap_range(), so the wrong
> sub-range gets mapped and the mr->offset / mr->size stored in the list
> no longer describe the memory actually returned by pci_iomap_range().
> Would it make sense to either reject offset/size that exceed ULONG_MAX
> here, or narrow the map API to unsigned long so the truncation is
> visible at the type level? In-tree idpf and ixd likely never hit this
> today, but the public signature invites the mismatch.
> > +	if (!va) {
> > +		pci_err(pdev, "Failed to map BAR%u region\n", bar_idx);
> > +		return false;
> > +	}
> [ ... ]