linux-kernel - Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 24 Mar 2021 14:55:15 +0100
From:   Roger Pau Monné <roger.pau@...rix.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
CC:     <linux-kernel@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
        "Mika Westerberg" <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andy@...nel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        <linux-gpio@...r.kernel.org>
Subject: 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 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.pau@...rix.com>
> > ---
> > Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>
> > Cc: Andy Shevchenko <andy@...nel.org>
> > Cc: Linus Walleij <linus.walleij@...aro.org>
> > Cc: linux-gpio@...r.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!