* [PATCH v2 0/2] intel/pinctrl: check some registers reads @ 2021-03-24 15:43 Roger Pau Monne 2021-03-24 15:43 ` [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence Roger Pau Monne 2021-03-24 15:43 ` [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region Roger Pau Monne 0 siblings, 2 replies; 6+ messages in thread From: Roger Pau Monne @ 2021-03-24 15:43 UTC (permalink / raw) To: linux-kernel Cc: xen-devel, Roger Pau Monne, Mika Westerberg, Andy Shevchenko, Linus Walleij, linux-gpio Hello, The following series adds some consistency checks to the values returned by some of the MMIO registers of the Intel pinctrl device. That done to avoid a crash when running as a PVH dom0. See patch #1 for more details. Thanks, Roger. Roger Pau Monne (2): intel/pinctrl: check REVID register value for device presence intel/pinctrl: check capability offset is between MMIO region drivers/pinctrl/intel/pinctrl-intel.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) -- 2.30.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence 2021-03-24 15:43 [PATCH v2 0/2] intel/pinctrl: check some registers reads Roger Pau Monne @ 2021-03-24 15:43 ` Roger Pau Monne 2021-03-24 17:01 ` Andy Shevchenko 2021-03-24 15:43 ` [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region Roger Pau Monne 1 sibling, 1 reply; 6+ messages in thread From: Roger Pau Monne @ 2021-03-24 15:43 UTC (permalink / raw) To: linux-kernel Cc: xen-devel, Roger Pau Monne, Andy Shevchenko, Mika Westerberg, Linus Walleij, linux-gpio Use the value read from the REVID register in order to check for the presence of the device. A read of all ones is treated as if the device is not present, and hence probing is ended. This fixes an issue when running as a Xen PVH dom0, where the ACPI DSDT table is provided unmodified to dom0 and hence contains the pinctrl devices, but the MMIO region(s) containing the device registers might not be mapped in the guest physical memory map if such region(s) are not exposed on a PCI device BAR or marked as reserved in the host memory map. Suggested-by: Andy Shevchenko <andy@kernel.org> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - New in this version. --- Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Andy Shevchenko <andy@kernel.org> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: linux-gpio@vger.kernel.org --- drivers/pinctrl/intel/pinctrl-intel.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 8085782cd8f9..59d13342caf6 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1493,6 +1493,8 @@ static int intel_pinctrl_probe(struct platform_device *pdev, /* Determine community features based on the revision */ value = readl(regs + REVID); + if (value == ~0u) + return -ENODATA; if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) { community->features |= PINCTRL_FEATURE_DEBOUNCE; community->features |= PINCTRL_FEATURE_1K_PD; -- 2.30.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence 2021-03-24 15:43 ` [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence Roger Pau Monne @ 2021-03-24 17:01 ` Andy Shevchenko 2021-03-25 8:36 ` Roger Pau Monné 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2021-03-24 17:01 UTC (permalink / raw) To: Roger Pau Monne Cc: linux-kernel, xen-devel, Andy Shevchenko, Mika Westerberg, Linus Walleij, linux-gpio On Wed, Mar 24, 2021 at 04:43:11PM +0100, Roger Pau Monne wrote: Thanks for a fix! My comments below. > Use the value read from the REVID register in order to check for the > presence of the device. A read of all ones is treated as if the device > is not present, and hence probing is ended. > > This fixes an issue when running as a Xen PVH dom0, where the ACPI > DSDT table is provided unmodified to dom0 and hence contains the > pinctrl devices, but the MMIO region(s) containing the device > registers might not be mapped in the guest physical memory map if such > region(s) are not exposed on a PCI device BAR or marked as reserved in > the host memory map. Any particular point that we can use in the Fixes tag? ... > Suggested-by: Andy Shevchenko <andy@kernel.org> Hmm... was it that address I have used? In any case I think my @linux.intel.com is better. ... > /* Determine community features based on the revision */ > value = readl(regs + REVID); > + if (value == ~0u) > + return -ENODATA; I think -ENODEV is more appropriate here. Also comment above should be adjusted to explain this check. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence 2021-03-24 17:01 ` Andy Shevchenko @ 2021-03-25 8:36 ` Roger Pau Monné 0 siblings, 0 replies; 6+ messages in thread From: Roger Pau Monné @ 2021-03-25 8:36 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-kernel, xen-devel, Andy Shevchenko, Mika Westerberg, Linus Walleij, linux-gpio On Wed, Mar 24, 2021 at 07:01:18PM +0200, Andy Shevchenko wrote: > On Wed, Mar 24, 2021 at 04:43:11PM +0100, Roger Pau Monne wrote: > > Thanks for a fix! My comments below. > > > Use the value read from the REVID register in order to check for the > > presence of the device. A read of all ones is treated as if the device > > is not present, and hence probing is ended. > > > > This fixes an issue when running as a Xen PVH dom0, where the ACPI > > DSDT table is provided unmodified to dom0 and hence contains the > > pinctrl devices, but the MMIO region(s) containing the device > > registers might not be mapped in the guest physical memory map if such > > region(s) are not exposed on a PCI device BAR or marked as reserved in > > the host memory map. > > Any particular point that we can use in the Fixes tag? Hm, I haven't seen those issues up until 91d898e51e60 ('pinctrl: intel: Convert capability list to features'), but the device wasn't working properly for sure, as the registers where not accessible, it just didn't lead to a kernel crash. > ... > > > Suggested-by: Andy Shevchenko <andy@kernel.org> > > Hmm... was it that address I have used? In any case I think my @linux.intel.com > is better. I just used the same as the one that's on the MAINTAINERS file, because I already had that n my Cc list. I can change to the @intel one if that's your preference. > ... > > > /* Determine community features based on the revision */ > > value = readl(regs + REVID); > > + if (value == ~0u) > > + return -ENODATA; > > I think -ENODEV is more appropriate here. > Also comment above should be adjusted to explain this check. Right, will change and send v3. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region 2021-03-24 15:43 [PATCH v2 0/2] intel/pinctrl: check some registers reads Roger Pau Monne 2021-03-24 15:43 ` [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence Roger Pau Monne @ 2021-03-24 15:43 ` Roger Pau Monne 2021-03-24 17:02 ` Andy Shevchenko 1 sibling, 1 reply; 6+ messages in thread From: Roger Pau Monne @ 2021-03-24 15:43 UTC (permalink / raw) To: linux-kernel Cc: xen-devel, Roger Pau Monne, Mika Westerberg, Andy Shevchenko, Linus Walleij, linux-gpio When parsing the capability list make sure the offset is between the MMIO region mapped in 'regs', or else the kernel hits a page fault. Adding the check is harmless, and prevents buggy or broken systems from crashing the kernel if the capability linked list is somehow broken. Fixes: 91d898e51e60 ('pinctrl: intel: Convert capability list to features') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Changes since v1: - Adjust commit message. --- Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Andy Shevchenko <andy@kernel.org> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: linux-gpio@vger.kernel.org --- drivers/pinctrl/intel/pinctrl-intel.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 59d13342caf6..d45a6994b2a3 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1481,16 +1481,22 @@ static int intel_pinctrl_probe(struct platform_device *pdev, for (i = 0; i < pctrl->ncommunities; i++) { struct intel_community *community = &pctrl->communities[i]; + struct resource *res; void __iomem *regs; + size_t size; u32 offset; u32 value; *community = pctrl->soc->communities[i]; - regs = devm_platform_ioremap_resource(pdev, community->barno); + regs = devm_platform_get_and_ioremap_resource(pdev, + community->barno, + &res); if (IS_ERR(regs)) return PTR_ERR(regs); + size = res->end - res->start; + /* Determine community features based on the revision */ value = readl(regs + REVID); if (value == ~0u) @@ -1521,6 +1527,12 @@ static int intel_pinctrl_probe(struct platform_device *pdev, break; } offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT; + if (offset >= size) { + dev_err(&pdev->dev, + "wrong capability offset: %#x\n", + offset); + return -ENOENT; + } } while (offset); dev_dbg(&pdev->dev, "Community%d features: %#08x\n", i, community->features); -- 2.30.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region 2021-03-24 15:43 ` [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region Roger Pau Monne @ 2021-03-24 17:02 ` Andy Shevchenko 0 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2021-03-24 17:02 UTC (permalink / raw) To: Roger Pau Monne Cc: linux-kernel, xen-devel, Mika Westerberg, Andy Shevchenko, Linus Walleij, linux-gpio On Wed, Mar 24, 2021 at 04:43:12PM +0100, Roger Pau Monne wrote: > When parsing the capability list make sure the offset is between the > MMIO region mapped in 'regs', or else the kernel hits a page fault. > > Adding the check is harmless, and prevents buggy or broken systems > from crashing the kernel if the capability linked list is somehow > broken. I don't think we need a dead code in the kernel. If you have a hardware to show this issue, I eagerly want to know this! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-25 8:37 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-24 15:43 [PATCH v2 0/2] intel/pinctrl: check some registers reads Roger Pau Monne 2021-03-24 15:43 ` [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence Roger Pau Monne 2021-03-24 17:01 ` Andy Shevchenko 2021-03-25 8:36 ` Roger Pau Monné 2021-03-24 15:43 ` [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region Roger Pau Monne 2021-03-24 17:02 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.