Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned

[Bjorn Helgaas <helgaas@kernel.org>]

[+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@gmail.com>
> 
> 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@gmail.com>
> ---
> 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@gmail.com
> 
> 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@gmail.com
> ---
>  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@gmail.com>
> 
>