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 0xffff (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
> 0xffff
> 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é <roger....@citrix.com>
> > ---
> > Cc: Mika Westerberg <mika.westerb...@linux.intel.com>
> > Cc: Andy Shevchenko <a...@kernel.org>
> > Cc: Linus Walleij <linus.wall...@linaro.org>
> > 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!
