Linux Virtualization - Sep 2021 - [PATCH v4 11/15] pci: Add pci_iomap_shared{,_range}

On Mon, Aug 30, 2021 at 05:23:17PM -0700, Andi Kleen
wrote:
> 
> On 8/30/2021 1:59 PM, Michael S. Tsirkin wrote:
> > 
> > > Or we can add _audited to the name. ioremap_shared_audited?
> > But it's not the mapping that has to be done in handled special
way.
> > It's any data we get from device, not all of it coming from IO,
e.g.
> > there's DMA and interrupts that all have to be validated.
> > Wouldn't you say that what is really wanted is just not running
> > unaudited drivers in the first place?
> 
> 
> Yes.
Then ... let's do just that?
> 
> > 
> > > And we've been avoiding that drivers can self declare
auditing, we've been
> > > trying to have a separate centralized list so that it's
easier to enforce
> > > and avoids any cut'n'paste mistakes.
> > > 
> > > -Andi
> > Now I'm confused. What is proposed here seems to be basically
that,
> > drivers need to declare auditing by replacing ioremap with
> > ioremap_shared.
> 
> Auditing is declared on the device model level using a central allow list.
Can we not have an init call allow list instead of, or in addition to, a
device allow list?
> But this cannot do anything to initcalls that run before probe,
Can't we extend module_init so init calls are validated against the
allow list?
> that's why
> an extra level of defense of ioremap opt-in is useful.
OK even assuming this, why is pci_iomap opt-in useful?
That never happens before probe - there's simply no pci_device then.
> But it's not the
> primary mechanism to declare a driver audited, that's the allow list.
The
> ioremap is just another mechanism to avoid having to touch a lot of legacy
> drivers.
> 
> If we agree on that then the original proposed semantics of
"ioremap_shared"
> may be acceptable?
> 
> -Andi
> 
It looks suspiciously like drivers self-declaring auditing to me which
we both seem to agree is undesirable. What exactly is the difference?

Or are you just trying to disable anything that runs before probe?
In that case I don't see a reason to touch pci drivers though.
These should be fine with just the device model list.

-- 
MST

>>>> And we've been avoiding that drivers can self declare
auditing, we've been
>>>> trying to have a separate centralized list so that it's
easier to enforce
>>>> and avoids any cut'n'paste mistakes.
>>>>
>>>> -Andi
>>> Now I'm confused. What is proposed here seems to be basically
that,
>>> drivers need to declare auditing by replacing ioremap with
>>> ioremap_shared.
>> Auditing is declared on the device model level using a central allow
list.
> Can we not have an init call allow list instead of, or in addition to, a
> device allow list?

That would be quite complicated and intrusive. In fact I'm not even sure 
how to do maintain something like this. There are a lot of needed 
initcalls, they would all need to be marked. How can we distinguish 
them? It would be a giant auditing project. And of course how would you 
prevent it from bitrotting?


Basically it would be hundreds of changes all over the tree, just to 
avoid two changes in virtio and MSI. Approach of just stopping the 
initcalls from doing bad things is much less intrusive.
>
>> But this cannot do anything to initcalls that run before probe,
> Can't we extend module_init so init calls are validated against the
> allow list?
See above.


Also the problem isn't really with modules (we rely on udev not loading 
them), but with builtin initcalls

>
>> that's why
>> an extra level of defense of ioremap opt-in is useful.
> OK even assuming this, why is pci_iomap opt-in useful?
> That never happens before probe - there's simply no pci_device then.

Hmm, yes that's true. I guess we can make it default to opt-in for 
pci_iomap.

It only really matters for device less ioremaps.
>
> It looks suspiciously like drivers self-declaring auditing to me which
> we both seem to agree is undesirable. What exactly is the difference?

Just allow listing the ioremaps is not self declaration because the 
device will still not initialize due to the central device filter. If 
you want to use it that has to be changed.

It's just an additional safety net to contain code running before probe.

>
> Or are you just trying to disable anything that runs before probe?

Well anything that could do dangerous host interactions (like processing 
ioremap data) A lot of things are harmless and can be allowed, or 
already blocked elsewhere (e.g. we have a IO port filter). This just 
handles the ioremap/MMIO case.
> In that case I don't see a reason to touch pci drivers though.
> These should be fine with just the device model list.

That won't stop initcalls.


-Andi

>