[+cc linux-mm for vmap page alignment checking question] On Wed, Aug 14, 2024 at 08:09:15PM +0800, Miao Wang via B4 Relay wrote: > From: Miao Wang <shankerwangmiao@xxxxxxxxx> > > When the IO resource given by _CRS method is not page aligned, especially > when the page size is larger than 4KB, serious problems will happen > because the misaligned address is passed down to pci_remap_iospace(), > then to vmap_page_range(), and finally to vmap_pte_range(), where the > length between addr and end is expected to be divisible by PAGE_SIZE, or > the loop will overrun till the pfn_none check fails. What does this problem look like to a user? Panic, oops, hang, warning backtrace? I assume this is not a regression, but maybe something you tripped over because of a BIOS defect? Does this need to be backported to stable kernels? It seems sort of weird to me that all those vmap_*_range() functions take the full page address (not a PFN) and depend on the addr/size being page-aligned, but they don't validate the alignment. But I'm not a VM person and I suppose there's a reason for passing the full address. But it does mean that other users of vmap_page_range() are also potentially susceptible to this issue, e.g., vmap(), vm_map_ram(), ioremap_page_range(), etc., so I'm not sure that acpi_pci_root_remap_iospace() is the best place to check the alignment. > Signed-off-by: Miao Wang <shankerwangmiao@xxxxxxxxx> > --- > Changes in v3: > - Adjust code formatting. > - Reword the commit message for further description of the possible reason > leading to misaligned IO resource addresses. > - Link to v2: https://lore.kernel.org/r/20240814-check_pci_probe_res-v2-1-a03c8c9b498b@xxxxxxxxx > > Changes in v2: > - Sorry for posting out the draft version in V1, fixed a silly compiling issue. > - Link to v1: https://lore.kernel.org/r/20240814-check_pci_probe_res-v1-1-122ee07821ab@xxxxxxxxx > --- > drivers/acpi/pci_root.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index d0bfb3706801..a425e93024f2 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -858,7 +858,7 @@ static void acpi_pci_root_validate_resources(struct device *dev, > } > } > > -static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode, > +static void acpi_pci_root_remap_iospace(struct acpi_device *device, > struct resource_entry *entry) > { > #ifdef PCI_IOBASE > @@ -868,7 +868,15 @@ static void acpi_pci_root_remap_iospace(struct fwnode_handle *fwnode, > resource_size_t length = resource_size(res); > unsigned long port; > > - if (pci_register_io_range(fwnode, cpu_addr, length)) > + if (!PAGE_ALIGNED(cpu_addr) || !PAGE_ALIGNED(length) || > + !PAGE_ALIGNED(pci_addr)) { > + dev_err(&device->dev, > + FW_BUG "I/O resource %pR or its offset %pa is not page aligned\n", > + res, &entry->offset); > + goto err; > + } > + > + if (pci_register_io_range(&device->fwnode, cpu_addr, length)) > goto err; This change verifies alignment for the ACPI case that leads to the pci_remap_iospace() -> vmap_page_range() -> vmap_pte_range() path, but there are others even in drivers/pci/, e.g., pci_remap_iospace() is also used in the DT path, where I suppose a defective DT could cause a similar issue. > port = pci_address_to_pio(cpu_addr); > @@ -910,7 +918,7 @@ int acpi_pci_probe_root_resources(struct acpi_pci_root_info *info) > else { > resource_list_for_each_entry_safe(entry, tmp, list) { > if (entry->res->flags & IORESOURCE_IO) > - acpi_pci_root_remap_iospace(&device->fwnode, > + acpi_pci_root_remap_iospace(device, > entry); > > if (entry->res->flags & IORESOURCE_DISABLED) > > --- > base-commit: 7c626ce4bae1ac14f60076d00eafe71af30450ba > change-id: 20240813-check_pci_probe_res-27e3e6df72b2 > > Best regards, > -- > Miao Wang <shankerwangmiao@xxxxxxxxx> > >
