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

On Sun, Aug 29, 2021 at 10:11:46PM -0700, Andi Kleen
wrote:
> 
> On 8/29/2021 3:26 PM, Michael S. Tsirkin wrote:
> > On Sun, Aug 29, 2021 at 09:17:53AM -0700, Andi Kleen wrote:
> > > Also I changing this single call really that bad? It's not
that we changing
> > > anything drastic here, just give the low level subsystem a better
hint about
> > > the intention. If you don't like the function name, could
make it an
> > > argument instead?
> > My point however is that the API should say that the
> > driver has been audited,
> 
> We have that status in the struct device. If you want to tie the ioremap to
> that we could define a ioremap_device() with a device argument and decide
> based on that.
But it's not the device that is audited. And it's not the device
that might be secure or insecure. It's the driver.
> 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?
> 
> > not that the mapping has been
> > done in some special way. For example the mapping can be
> > in some kind of wrapper, not directly in the driver.
> > However you want the driver validated, not the wrapper.
> > 
> > Here's an idea:
> 
> 
> I don't think magic differences of API behavior based on some define
are a
> good idea.? That's easy to miss.
Well ... my point is that actually there is no difference in API
behaviour. the map is the same map, exactly same data goes to device. If
anything any non-shared map is special in that encrypted data goes to
device.
> 
> That's a "COME FROM" in API design.
> 
> Also it wouldn't handle the case that a driver has both private and
shared
> ioremaps, e.g. for BIOS structures.
Hmm. Interesting.  It's bios maps that are unusual and need to be private
though ...
> 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.

-- 
MST

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.

>
>> 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.

But this cannot do anything to initcalls that run before probe, that's 
why an extra level of defense of ioremap opt-in is useful. 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

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