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 18:57:12 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Roger Pau Monné <roger.pau@...rix.com>
Cc:     linux-kernel@...r.kernel.org, xen-devel@...ts.xenproject.org,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        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 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