linux-kernel - Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

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]

Message-ID: <20190408165001.GD7872@fuggles.cambridge.arm.com>
Date:   Mon, 8 Apr 2019 17:50:01 +0100
From:   Will Deacon <will.deacon@....com>
To:     John Garry <john.garry@...wei.com>
Cc:     Guenter Roeck <linux@...ck-us.net>,
        Bjorn Helgaas <helgaas@...nel.org>,
        Hardware Monitoring <linux-hwmon@...r.kernel.org>,
        wangkefeng.wang@...wei.com, lorenzo.pieralisi@....com,
        arnd@...db.de, rafael@...nel.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linuxarm@...wei.com,
        andy.shevchenko@...il.com, catalin.marinas@....com, bp@...e.de,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered
 CPU MMIO regions

On Mon, Apr 08, 2019 at 05:35:51PM +0100, John Garry wrote:
> On 08/04/2019 14:47, Guenter Roeck wrote:
> > > > > > FC patch 1/4 ("resource: Request IO port regions from children
> > > > > > of ioport_resource").
> > > > > 
> > > > > Maybe I'm missing something, but on x86, drivers like f71882fg do not
> > > > > crash the system because inb() *never* causes a crash.
> > > > > 
> > > > > If you want to build that driver for ARM, I think you need to make
> > > > > sure that inb() on ARM also *never* causes a crash.  I don't think
> > > > > changing f71882fg and all the similar drivers is the right answer.
> > > > > 
> > > > 
> > > > Agreed. As I had mentioned earlier, the driver changes are orthogonal:
> > > > the drivers should request the IO region before accessing it, primarily
> > > > to avoid conflicting accesses by multiple drivers in parallel. For
> > > > example, the f71882fg driver supports chips which implement hardware
> > > > monitoring as well as watchdog functionality, and both the hwmon
> > > > and the watchdog driver may try to access the io space.
> > > > 
> > > > If and how the system ensures that the IO region exists and/or that
> > > > inb() always succeeds is a different question. I would prefer a less
> > > > complex solution than the one suggested here, but that is my personal
> > > > opionion.
> > > 
> > > Hi Guenter,
> > > 
> > > I have a question about these super-IO accesses:
> > > 
> > > To me, it's not good that these hwmon, watchdog, gpio, etc drivers
> > > make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
> > > request_muxed_region() call).
> > > 
> > > The issue I see is that on an arm, IO space for some other device may
> > > be mapped in this region, so it would not be right for these drivers
> > > to access those same regions.
> > > 
> > Yes, but then there _could_ be some arm or arm64 device supporting one
> > of those chips,
> > so we can not just add something like "depends on !(ARM || ARM64)".
> 
> This looks like what has been added for PPC in commmit 746cdfbf01c0.
> 
> However, agreed, it's not a good approach.
> 
> > 
> > > Is there any other platform check which can be made to ensure that
> > > accesses these super-IO ports is appropriate?
> > > 
> > 
> > Not that I know of. It would make some sense to provide API functions
> > for Super-IO accesses, but that would be a lot of work, and I guess
> > it isn't really valuable enough for anyone to pick up and do.
> > 
> > Normally, if you have such a system, the respective drivers should not be
> > built. After all, this isn't the only instance where drivers
> > unconditionally
> > access some io region, no matter if the underlying hardware exists or not.
> > The only real defense against that is to not build those drivers into
> > a given kernel.
> 
> If we're going to support a multi-plaform kernel for a given arch, then we
> can't always avoid it.
> 
> It seems that the only solution on the table now is to discard these IO port
> accesses on arm64 when the IO port are not mapped.

Hmm, how are you going to achieve that? I'm not sure we can guarantee a
synchronous abort, so I'd be nervous about anything that tries to handle
the exception after making the unmapped access.

Will