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

From: Will Deacon <will.deacon@arm.com>
To: John Garry <john.garry@huawei.com>
Cc: Guenter Roeck <linux@roeck-us.net>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Hardware Monitoring <linux-hwmon@vger.kernel.org>,
	wangkefeng.wang@huawei.com, lorenzo.pieralisi@arm.com,
	arnd@arndb.de, rafael@kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, linuxarm@huawei.com,
	andy.shevchenko@gmail.com, catalin.marinas@arm.com, bp@suse.de,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions
Date: Mon, 8 Apr 2019 17:50:01 +0100	[thread overview]
Message-ID: <20190408165001.GD7872@fuggles.cambridge.arm.com> (raw)
In-Reply-To: <efff6e58-b48d-f168-ed92-1714f5a08292@huawei.com>

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