Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Thu, Mar 25, 2021 at 09:46:46AM +0100, Roger Pau Monné wrote: > On Wed, Mar 24, 2021 at 06:57:12PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote: > > > On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote: > > > > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote: > > > > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote: ... > > > > Unfortunately it does not expose PCI configuration space. > > > > > > Are those regions supposed to be marked as reserved in the memory map, > > > or that's left to the discretion of the hardware vendor? > > > > I didn't get. The OS doesn't see them and an internal backbone simply drops > > any > > IO access to that region. > > I'm not sure I understand the above reply. My question was whether the > MMIO regions used by the pinctrl device (as fetched from the ACPI DSDT > table) are supposed belong to regions marked as RESERVED in the > firmware memory map (ie: either the e820 or the EFI one). I don't actually know. I guess it should be done in order to have ACPI device a possibility to claim the resource. > > > > > Doing something like pci_device_is_present would require a register > > > > > that we know will never return ~0 unless the device is not present. As > > > > > said above, maybe we could use REVID to that end? > > > > > > > > Yes, that's good, see above. > > > > > > > > WRT capabilities, if we crash we will see the report immediately on the > > > > hardware which has such an issue. (It's quite unlikely we will ever > > > > have one, > > > > that's why I consider it's not critical) > > > > > > I would rather prefer to not crash, because I think the kernel should > > > only resort to crashing when there's no alternative, and here it's > > > perfectly fine to just print an error message and don't load the > > > driver. > > > > Are we speaking about real hardware that has an issue? I eagerly want to > > know > > what is that beast. > > OK, I'm not going to resend this anymore. I'm happy with just getting > the first patch in. > > I think you trust the hardware more that I would do, and I also think > the check added here is very minimal an unintrusive and serves as a > way to sanitize the data fetched from the hardware in order to prevent > a kernel page fault if such data turns out to be wrong. > > Taking a reactive approach of requiring a broken piece of hardware to > exist in order to sanitize a fetched value seems too risky. I could > add a WARN_ON or similar if you want some kind of splat that's very > noticeable when this goes wrong but that doesn't end up in a fatal > kernel page fault. You found the issue anyway as long as you had a crash, so current code already proved that it does it work perfectly. Since I know what hardware this driver is for, I can assure you, that it will be quite unlikely to have wrong data in the capability register. The data sheet is crystal clear about the register's contents: on real hardware it must be present and be set to a sane value. -- With Best Regards, Andy Shevchenko
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 06:57:12PM +0200, Andy Shevchenko wrote: > On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote: > > On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote: > > > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote: > > > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote: > > > > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote: > > > > > Moreover, it seems you are bailing out and basically denying driver > > > > > to load. > > > > > This does look that capability is simply the first register that > > > > > blows the setup. > > > > > I think you have to fix something into Xen to avoid loading these > > > > > drivers or > > > > > check with something like pci_device_is_present() approach. > > > > > > > > Is there a backing PCI device BAR for those MMIO regions that the > > > > pinctrl driver is trying to access? AFAICT those regions are only > > > > reported in the ACPI DSDT table on the _CRS method of the object (at > > > > least on my system). > > > > > > Unfortunately it does not expose PCI configuration space. > > > > Are those regions supposed to be marked as reserved in the memory map, > > or that's left to the discretion of the hardware vendor? > > I didn't get. The OS doesn't see them and an internal backbone simply drops > any > IO access to that region. I'm not sure I understand the above reply. My question was whether the MMIO regions used by the pinctrl device (as fetched from the ACPI DSDT table) are supposed belong to regions marked as RESERVED in the firmware memory map (ie: either the e820 or the EFI one). > > > > Doing something like pci_device_is_present would require a register > > > > that we know will never return ~0 unless the device is not present. As > > > > said above, maybe we could use REVID to that end? > > > > > > Yes, that's good, see above. > > > > > > WRT capabilities, if we crash we will see the report immediately on the > > > hardware which has such an issue. (It's quite unlikely we will ever have > > > one, > > > that's why I consider it's not critical) > > > > I would rather prefer to not crash, because I think the kernel should > > only resort to crashing when there's no alternative, and here it's > > perfectly fine to just print an error message and don't load the > > driver. > > Are we speaking about real hardware that has an issue? I eagerly want to know > what is that beast. OK, I'm not going to resend this anymore. I'm happy with just getting the first patch in. I think you trust the hardware more that I would do, and I also think the check added here is very minimal an unintrusive and serves as a way to sanitize the data fetched from the hardware in order to prevent a kernel page fault if such data turns out to be wrong. Taking a reactive approach of requiring a broken piece of hardware to exist in order to sanitize a fetched value seems too risky. I could add a WARN_ON or similar if you want some kind of splat that's very noticeable when this goes wrong but that doesn't end up in a fatal kernel page fault. Thanks, Roger.
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote: > On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote: > > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote: > > > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote: > > > > Moreover, it seems you are bailing out and basically denying driver to > > > > load. > > > > This does look that capability is simply the first register that blows > > > > the setup. > > > > I think you have to fix something into Xen to avoid loading these > > > > drivers or > > > > check with something like pci_device_is_present() approach. > > > > > > Is there a backing PCI device BAR for those MMIO regions that the > > > pinctrl driver is trying to access? AFAICT those regions are only > > > reported in the ACPI DSDT table on the _CRS method of the object (at > > > least on my system). > > > > Unfortunately it does not expose PCI configuration space. > > Are those regions supposed to be marked as reserved in the memory map, > or that's left to the discretion of the hardware vendor? I didn't get. The OS doesn't see them and an internal backbone simply drops any IO access to that region. > > > Doing something like pci_device_is_present would require a register > > > that we know will never return ~0 unless the device is not present. As > > > said above, maybe we could use REVID to that end? > > > > Yes, that's good, see above. > > > > WRT capabilities, if we crash we will see the report immediately on the > > hardware which has such an issue. (It's quite unlikely we will ever have > > one, > > that's why I consider it's not critical) > > I would rather prefer to not crash, because I think the kernel should > only resort to crashing when there's no alternative, and here it's > perfectly fine to just print an error message and don't load the > driver. Are we speaking about real hardware that has an issue? I eagerly want to know what is that beast. > IMO I would rather boot without pinctrl than get a panic if > it turns out pinctrl capabilities list is somehow corrupted. Again, do you have a hardware that does this? > It's a > long shot, but the check added in order to prevent this scenario is > minimal. > In any case I will send a new version with the REVID check and this > current patch. Okay, let's continue there, but I'm pessimistic about accepting this patch. -- With Best Regards, Andy Shevchenko
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote: > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote: > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote: > > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote: > > > Moreover, it seems you are bailing out and basically denying driver to > > > load. > > > This does look that capability is simply the first register that blows > > > the setup. > > > I think you have to fix something into Xen to avoid loading these drivers > > > or > > > check with something like pci_device_is_present() approach. > > > > Is there a backing PCI device BAR for those MMIO regions that the > > pinctrl driver is trying to access? AFAICT those regions are only > > reported in the ACPI DSDT table on the _CRS method of the object (at > > least on my system). > > Unfortunately it does not expose PCI configuration space. Are those regions supposed to be marked as reserved in the memory map, or that's left to the discretion of the hardware vendor? > > Doing something like pci_device_is_present would require a register > > that we know will never return ~0 unless the device is not present. As > > said above, maybe we could use REVID to that end? > > Yes, that's good, see above. > > WRT capabilities, if we crash we will see the report immediately on the > hardware which has such an issue. (It's quite unlikely we will ever have one, > that's why I consider it's not critical) I would rather prefer to not crash, because I think the kernel should only resort to crashing when there's no alternative, and here it's perfectly fine to just print an error message and don't load the driver. IMO I would rather boot without pinctrl than get a panic if it turns out pinctrl capabilities list is somehow corrupted. It's a long shot, but the check added in order to prevent this scenario is minimal. In any case I will send a new version with the REVID check and this current patch. Thanks, Roger.
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote: > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote: ... > What could be done is check whether reading REVID returns ~0 and exit > at that point, if ~0 will never be a valid value returned by that > register. I think that should be a separate patch however. Sounds good to me. > > Moreover, it seems you are bailing out and basically denying driver to load. > > This does look that capability is simply the first register that blows the > > setup. > > I think you have to fix something into Xen to avoid loading these drivers or > > check with something like pci_device_is_present() approach. > > Is there a backing PCI device BAR for those MMIO regions that the > pinctrl driver is trying to access? AFAICT those regions are only > reported in the ACPI DSDT table on the _CRS method of the object (at > least on my system). Unfortunately it does not expose PCI configuration space. > Doing something like pci_device_is_present would require a register > that we know will never return ~0 unless the device is not present. As > said above, maybe we could use REVID to that end? Yes, that's good, see above. WRT capabilities, if we crash we will see the report immediately on the hardware which has such an issue. (It's quite unlikely we will ever have one, that's why I consider it's not critical) -- With Best Regards, Andy Shevchenko
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 24, 2021 at 01:31:18PM +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.
> >
> > This fault has been seen when running as a Xen PVH dom0, which doesn't
> > have the MMIO regions mapped into the domain physical memory map,
> > despite having the device reported in the ACPI DSDT table. This
> > results in reporting a capability offset of 0x (because the kernel
> > is accessing unpopulated memory), and such offset is outside of the
> > mapped region.
> >
> > Adding the check is harmless, and prevents buggy or broken systems
> > from crashing the kernel if the MMIO region is not properly reported.
>
> Thanks for the report.
>
> Looking into the code I would like rather see the explicit comparison to
> 0x
> or ~0 against entire register b/c it's (one of) standard way of devices to
> tell
> that something is not supported.
That can be done also. I think what I've proposed is slightly more
robust, as it will prevent a kernel page fault if somehow the offset
to the next capability is below ~0, but past the end of the MMIO
region. Unlikely I know, but it's not worth a kernel panic.
What could be done is check whether reading REVID returns ~0 and exit
at that point, if ~0 will never be a valid value returned by that
register. I think that should be a separate patch however.
> Moreover, it seems you are bailing out and basically denying driver to load.
> This does look that capability is simply the first register that blows the
> setup.
> I think you have to fix something into Xen to avoid loading these drivers or
> check with something like pci_device_is_present() approach.
Is there a backing PCI device BAR for those MMIO regions that the
pinctrl driver is trying to access? AFAICT those regions are only
reported in the ACPI DSDT table on the _CRS method of the object (at
least on my system).
Doing something like pci_device_is_present would require a register
that we know will never return ~0 unless the device is not present. As
said above, maybe we could use REVID to that end?
> > Fixes: 91d898e51e60 ('pinctrl: intel: Convert capability list to features')
> > Signed-off-by: Roger Pau Monné
> > ---
> > Cc: Mika Westerberg
> > Cc: Andy Shevchenko
> > Cc: Linus Walleij
> > Cc: linux-g...@vger.kernel.org
> > ---
> > Resend because I've missed adding the maintainers, sorry for the spam.
>
> I have a script to make it easier:
> https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
Thanks!
Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
On Wed, Mar 24, 2021 at 01:31:18PM +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.
>
> This fault has been seen when running as a Xen PVH dom0, which doesn't
> have the MMIO regions mapped into the domain physical memory map,
> despite having the device reported in the ACPI DSDT table. This
> results in reporting a capability offset of 0x (because the kernel
> is accessing unpopulated memory), and such offset is outside of the
> mapped region.
>
> Adding the check is harmless, and prevents buggy or broken systems
> from crashing the kernel if the MMIO region is not properly reported.
Thanks for the report.
Looking into the code I would like rather see the explicit comparison to 0x
or ~0 against entire register b/c it's (one of) standard way of devices to tell
that something is not supported.
Moreover, it seems you are bailing out and basically denying driver to load.
This does look that capability is simply the first register that blows the
setup.
I think you have to fix something into Xen to avoid loading these drivers or
check with something like pci_device_is_present() approach.
> Fixes: 91d898e51e60 ('pinctrl: intel: Convert capability list to features')
> Signed-off-by: Roger Pau Monné
> ---
> Cc: Mika Westerberg
> Cc: Andy Shevchenko
> Cc: Linus Walleij
> Cc: linux-g...@vger.kernel.org
> ---
> Resend because I've missed adding the maintainers, sorry for the spam.
I have a script to make it easier:
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
--
With Best Regards,
Andy Shevchenko
[PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region
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.
This fault has been seen when running as a Xen PVH dom0, which doesn't
have the MMIO regions mapped into the domain physical memory map,
despite having the device reported in the ACPI DSDT table. This
results in reporting a capability offset of 0x (because the kernel
is accessing unpopulated memory), and such offset is outside of the
mapped region.
Adding the check is harmless, and prevents buggy or broken systems
from crashing the kernel if the MMIO region is not properly reported.
Fixes: 91d898e51e60 ('pinctrl: intel: Convert capability list to features')
Signed-off-by: Roger Pau Monné
---
Cc: Mika Westerberg
Cc: Andy Shevchenko
Cc: Linus Walleij
Cc: linux-g...@vger.kernel.org
---
Resend because I've missed adding the maintainers, sorry for the spam.
---
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 8085782cd8f9..bc8b990d8021 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 & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
@@ -1519,6 +1525,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
