* [PATCH v3] ACPI: PCI: check if the root io space is page aligned @ 2024-08-14 12:09 Miao Wang via B4 Relay 2024-08-14 16:37 ` Bjorn Helgaas 0 siblings, 1 reply; 6+ messages in thread From: Miao Wang via B4 Relay @ 2024-08-14 12:09 UTC (permalink / raw) To: Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown Cc: linux-pci, linux-acpi, Miao Wang 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. 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; 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> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned 2024-08-14 12:09 [PATCH v3] ACPI: PCI: check if the root io space is page aligned Miao Wang via B4 Relay @ 2024-08-14 16:37 ` Bjorn Helgaas 2024-08-15 4:28 ` Miao Wang 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Helgaas @ 2024-08-14 16:37 UTC (permalink / raw) To: shankerwangmiao Cc: Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown, linux-pci, linux-acpi, linux-mm [+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> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned 2024-08-14 16:37 ` Bjorn Helgaas @ 2024-08-15 4:28 ` Miao Wang 2024-08-21 4:43 ` Miao Wang 0 siblings, 1 reply; 6+ messages in thread From: Miao Wang @ 2024-08-15 4:28 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown, linux-pci, linux-acpi, linux-mm Hi, > 2024年8月15日 00:37，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? Panic, or actually BUG in vmap_pte_range() at the !pte_none(ptep_get(pte)) check, since misaligned addresses will cause the loop in vmap_pte_range overrun and finally reach one of the already mapped pages. This happens on a LS2k2000 machine, the buggy firmware of which declares the IO space of the PCI root controller as follows: QWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, 0x0000000000000000, // Granularity 0x0000000000004000, // Range Minimum 0x0000000000009FFF, // Range Maximum 0x000000FDFC000000, // Translation Offset 0x0000000000006000, // Length ,, , TypeStatic, DenseTranslation) At first, I thought there might be some overlapping address spaces. But when I added some debug output in vmap_page_range(), I realized that it was because a loop overrun. Normally, loongarch64 kernel is compiled using 16K page size, and thus the length here is not page aligned. I tested my patch using a virtual machine with a deliberately modified DSDT table to reproduce this issue. > 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. Ah, I also have this question. > > 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. My first idea was that the misaligned address is introduced from DSDT table and the check would be better to be done inside the ACPI system. However, lets wait for replies from linux-mm to decide where should be the best place. > >> 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned 2024-08-15 4:28 ` Miao Wang @ 2024-08-21 4:43 ` Miao Wang 2024-09-18 17:14 ` Miao Wang 0 siblings, 1 reply; 6+ messages in thread From: Miao Wang @ 2024-08-21 4:43 UTC (permalink / raw) To: Bjorn Helgaas, Andrew Morton Cc: Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown, linux-pci, linux-acpi, linux-mm Sorry for directly looping Andrew in. I think Andrew may be familiar with the code in question. Some backgrounds: Mis-aligned addresses from ACPI table can pass along pci_remap_iospace() -> vmap_page_range() -> vmap_pte_range() path, leading to a loop overrun in vmap_pte_range(). Bjorn and I wonder why all those vmap_*_range() functions don't validate the alignment, assuming the addresses page-aligned. We want to know the best place to do this check. > 2024年8月15日 12:28，Miao Wang <shankerwangmiao@gmail.com> 写道： > > Hi, > >> 2024年8月15日 00:37，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? > > Panic, or actually BUG in vmap_pte_range() at the !pte_none(ptep_get(pte)) > check, since misaligned addresses will cause the loop in vmap_pte_range > overrun and finally reach one of the already mapped pages. This happens on > a LS2k2000 machine, the buggy firmware of which declares the IO space of > the PCI root controller as follows: > > QWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, > 0x0000000000000000, // Granularity > 0x0000000000004000, // Range Minimum > 0x0000000000009FFF, // Range Maximum > 0x000000FDFC000000, // Translation Offset > 0x0000000000006000, // Length > ,, , TypeStatic, DenseTranslation) > > At first, I thought there might be some overlapping address spaces. But when > I added some debug output in vmap_page_range(), I realized that it was > because a loop overrun. > > Normally, loongarch64 kernel is compiled using 16K page size, and thus the > length here is not page aligned. I tested my patch using a virtual machine > with a deliberately modified DSDT table to reproduce this issue. > >> 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. > Ah, I also have this question. >> >> 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. > My first idea was that the misaligned address is introduced from DSDT > table and the check would be better to be done inside the ACPI system. > However, lets wait for replies from linux-mm to decide where should be > the best place. >> >>> 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned 2024-08-21 4:43 ` Miao Wang @ 2024-09-18 17:14 ` Miao Wang 2024-11-21 15:01 ` Miao Wang 0 siblings, 1 reply; 6+ messages in thread From: Miao Wang @ 2024-09-18 17:14 UTC (permalink / raw) To: Bjorn Helgaas Cc: Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown, linux-pci, linux-acpi, linux-mm > 2024年8月21日 12:43，Miao Wang <shankerwangmiao@gmail.com> 写道： > > Sorry for directly looping Andrew in. I think Andrew may be familiar with > the code in question. > > Some backgrounds: Mis-aligned addresses from ACPI table can pass along > pci_remap_iospace() -> vmap_page_range() -> vmap_pte_range() path, leading to a > loop overrun in vmap_pte_range(). Bjorn and I wonder why all those > vmap_*_range() functions don't validate the alignment, assuming the addresses > page-aligned. We want to know the best place to do this check. > >> 2024年8月15日 12:28，Miao Wang <shankerwangmiao@gmail.com> 写道： >> >> Hi, >> >>> 2024年8月15日 00:37，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? >> >> Panic, or actually BUG in vmap_pte_range() at the !pte_none(ptep_get(pte)) >> check, since misaligned addresses will cause the loop in vmap_pte_range >> overrun and finally reach one of the already mapped pages. This happens on >> a LS2k2000 machine, the buggy firmware of which declares the IO space of >> the PCI root controller as follows: >> >> QWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, EntireRange, >> 0x0000000000000000, // Granularity >> 0x0000000000004000, // Range Minimum >> 0x0000000000009FFF, // Range Maximum >> 0x000000FDFC000000, // Translation Offset >> 0x0000000000006000, // Length >> ,, , TypeStatic, DenseTranslation) >> >> At first, I thought there might be some overlapping address spaces. But when >> I added some debug output in vmap_page_range(), I realized that it was >> because a loop overrun. >> >> Normally, loongarch64 kernel is compiled using 16K page size, and thus the >> length here is not page aligned. I tested my patch using a virtual machine >> with a deliberately modified DSDT table to reproduce this issue. >> >>> 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. >> Ah, I also have this question. >>> >>> 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. >> My first idea was that the misaligned address is introduced from DSDT >> table and the check would be better to be done inside the ACPI system. >> However, lets wait for replies from linux-mm to decide where should be >> the best place. It seems that there is no reply from linux-mm about this. I believe even if there might be a better place for the mm subsystem to check the alignment, it is still necessary to do the check here, because details about which ACPI entry is causing the problem is only available here. If in the future, we would developed another better place to do the alignment check, we may refactor the code here. >>> >>>> 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> > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] ACPI: PCI: check if the root io space is page aligned 2024-09-18 17:14 ` Miao Wang @ 2024-11-21 15:01 ` Miao Wang 0 siblings, 0 replies; 6+ messages in thread From: Miao Wang @ 2024-11-21 15:01 UTC (permalink / raw) To: Bjorn Helgaas, Ilpo Järvinen, Bjorn Helgaas, Rafael J. Wysocki, Len Brown Cc: linux-pci, linux-acpi, linux-mm > It seems that there is no reply from linux-mm about this. I believe even if > there might be a better place for the mm subsystem to check the alignment, > it is still necessary to do the check here, because details about which ACPI > entry is causing the problem is only available here. If in the future, we would > developed another better place to do the alignment check, we may refactor the > code here. Hi, all Ping on this patch. > >>>> >>>>> 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-21 15:01 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-08-14 12:09 [PATCH v3] ACPI: PCI: check if the root io space is page aligned Miao Wang via B4 Relay 2024-08-14 16:37 ` Bjorn Helgaas 2024-08-15 4:28 ` Miao Wang 2024-08-21 4:43 ` Miao Wang 2024-09-18 17:14 ` Miao Wang 2024-11-21 15:01 ` Miao Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox