vpci: deferral of register write until p2m changes are done

vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

Hello,

While doing the recent vPCI fixes and also working on SR-IOV support
I've been thinking about how vPCI handles writes to PCI registers that
imply modifications to the p2m for PVH Dom0.

When memory decoding or ROM BARs are enabled Xen performs the
following flow:

1. Create a rangeset with the memory regions that need to be
mapped/unmapped.
2. Block the vCPU and perform the p2m changes in a preemptive way.
3. After the p2m changes have been applied (or in case of error) write
to the register in order to enable/disable memory decoding or the ROM
BAR and mark the BARs as enabled.

I'm unsure about the benefit of deferring the register write (step 3)
for a PVH Dom0, so I would like to perform the register write before
applying the changes to the p2m.

This would simplify the logic of the deferred p2m operations by not
having to store the register value, and would also remove the ugly
hack that we currently have for ROM BARs where vPCI fabricates a dummy
command register just to signal whether the operation is a map or
unmap.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 28.11.18 at 11:09, <roger.pau@citrix.com> wrote:
> Hello,
> 
> While doing the recent vPCI fixes and also working on SR-IOV support
> I've been thinking about how vPCI handles writes to PCI registers that
> imply modifications to the p2m for PVH Dom0.
> 
> When memory decoding or ROM BARs are enabled Xen performs the
> following flow:
> 
> 1. Create a rangeset with the memory regions that need to be
> mapped/unmapped.
> 2. Block the vCPU and perform the p2m changes in a preemptive way.
> 3. After the p2m changes have been applied (or in case of error) write
> to the register in order to enable/disable memory decoding or the ROM
> BAR and mark the BARs as enabled.
> 
> I'm unsure about the benefit of deferring the register write (step 3)
> for a PVH Dom0, so I would like to perform the register write before
> applying the changes to the p2m.

As expressed while reviewing respective patches, I'm not sure either.
Being not sure, putting ourselves on the safe side by disabling decode
early and enabling decode late seems best to me though. Any
deviation from this would imo require a conclusive discussion of why
it is safe. In the interest of later enabling of the code for DomU, any
such discussion should, as far as possible, avoid argumentation along
the Dom0-only line.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

On Wed, Nov 28, 2018 at 06:01:12AM -0700, Jan Beulich wrote:
> >>> On 28.11.18 at 11:09, <roger.pau@citrix.com> wrote:
> > Hello,
> > 
> > While doing the recent vPCI fixes and also working on SR-IOV support
> > I've been thinking about how vPCI handles writes to PCI registers that
> > imply modifications to the p2m for PVH Dom0.
> > 
> > When memory decoding or ROM BARs are enabled Xen performs the
> > following flow:
> > 
> > 1. Create a rangeset with the memory regions that need to be
> > mapped/unmapped.
> > 2. Block the vCPU and perform the p2m changes in a preemptive way.
> > 3. After the p2m changes have been applied (or in case of error) write
> > to the register in order to enable/disable memory decoding or the ROM
> > BAR and mark the BARs as enabled.
> > 
> > I'm unsure about the benefit of deferring the register write (step 3)
> > for a PVH Dom0, so I would like to perform the register write before
> > applying the changes to the p2m.
> 
> As expressed while reviewing respective patches, I'm not sure either.
> Being not sure, putting ourselves on the safe side by disabling decode
> early and enabling decode late seems best to me though. Any
> deviation from this would imo require a conclusive discussion of why
> it is safe.

Right. So there are two different cases here:

 - Mapping: enabling memory decoding before mapping should have no
   effect on the guest, since the p2m entries for the BARs won't be
   set.
 - Unmapping: disabling the memory decoding bit before unmapping could
   allow the guest to access RAM or other MMIO regions that are
   exposed once the BARs are no longer mapped?

> In the interest of later enabling of the code for DomU, any
> such discussion should, as far as possible, avoid argumentation along
> the Dom0-only line.

My plan is that DomUs won't be allowed to toggle the memory decoding
bit, and it's going to be always enabled, like it's currently done for
pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
going to trigger a change to the p2m (map or unmap) but the command
register will always have the memory decoding bit enabled.

Hence I think it's safe and easier to implement to perform the
register write ahead of the p2m changes.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
> On Wed, Nov 28, 2018 at 06:01:12AM -0700, Jan Beulich wrote:
>> >>> On 28.11.18 at 11:09, <roger.pau@citrix.com> wrote:
>> > Hello,
>> > 
>> > While doing the recent vPCI fixes and also working on SR-IOV support
>> > I've been thinking about how vPCI handles writes to PCI registers that
>> > imply modifications to the p2m for PVH Dom0.
>> > 
>> > When memory decoding or ROM BARs are enabled Xen performs the
>> > following flow:
>> > 
>> > 1. Create a rangeset with the memory regions that need to be
>> > mapped/unmapped.
>> > 2. Block the vCPU and perform the p2m changes in a preemptive way.
>> > 3. After the p2m changes have been applied (or in case of error) write
>> > to the register in order to enable/disable memory decoding or the ROM
>> > BAR and mark the BARs as enabled.
>> > 
>> > I'm unsure about the benefit of deferring the register write (step 3)
>> > for a PVH Dom0, so I would like to perform the register write before
>> > applying the changes to the p2m.
>> 
>> As expressed while reviewing respective patches, I'm not sure either.
>> Being not sure, putting ourselves on the safe side by disabling decode
>> early and enabling decode late seems best to me though. Any
>> deviation from this would imo require a conclusive discussion of why
>> it is safe.
> 
> Right. So there are two different cases here:
> 
>  - Mapping: enabling memory decoding before mapping should have no
>    effect on the guest, since the p2m entries for the BARs won't be
>    set.

Depends on whether this is a first time map, or a movement. In the
latter case it's the opposite of unmapping.

>  - Unmapping: disabling the memory decoding bit before unmapping could
>    allow the guest to access RAM or other MMIO regions that are
>    exposed once the BARs are no longer mapped?

Disable before unmap is what we want for safety, so I'm not sure
if you've simply mis-typed your reply. And disabling early is also in
line with your desire of doing the cmd register writes first.

>> In the interest of later enabling of the code for DomU, any
>> such discussion should, as far as possible, avoid argumentation along
>> the Dom0-only line.
> 
> My plan is that DomUs won't be allowed to toggle the memory decoding
> bit, and it's going to be always enabled, like it's currently done for
> pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
> going to trigger a change to the p2m (map or unmap) but the command
> register will always have the memory decoding bit enabled.

But this isn't entirely correct, even if we've got away with this
so far. But we're mostly considering well-behaved guests and
devices. What if one actually triggers bus activity in parallel to
a BAR change?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
> > On Wed, Nov 28, 2018 at 06:01:12AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.18 at 11:09, <roger.pau@citrix.com> wrote:
> >> > Hello,
> >> > 
> >> > While doing the recent vPCI fixes and also working on SR-IOV support
> >> > I've been thinking about how vPCI handles writes to PCI registers that
> >> > imply modifications to the p2m for PVH Dom0.
> >> > 
> >> > When memory decoding or ROM BARs are enabled Xen performs the
> >> > following flow:
> >> > 
> >> > 1. Create a rangeset with the memory regions that need to be
> >> > mapped/unmapped.
> >> > 2. Block the vCPU and perform the p2m changes in a preemptive way.
> >> > 3. After the p2m changes have been applied (or in case of error) write
> >> > to the register in order to enable/disable memory decoding or the ROM
> >> > BAR and mark the BARs as enabled.
> >> > 
> >> > I'm unsure about the benefit of deferring the register write (step 3)
> >> > for a PVH Dom0, so I would like to perform the register write before
> >> > applying the changes to the p2m.
> >> 
> >> As expressed while reviewing respective patches, I'm not sure either.
> >> Being not sure, putting ourselves on the safe side by disabling decode
> >> early and enabling decode late seems best to me though. Any
> >> deviation from this would imo require a conclusive discussion of why
> >> it is safe.
> > 
> > Right. So there are two different cases here:
> > 
> >  - Mapping: enabling memory decoding before mapping should have no
> >    effect on the guest, since the p2m entries for the BARs won't be
> >    set.
> 
> Depends on whether this is a first time map, or a movement. In the
> latter case it's the opposite of unmapping.

vPCI doesn't support BAR movements with memory decoding enabled ATM,
in order to move the position of a BAR memory decoding must be
disabled.

> >  - Unmapping: disabling the memory decoding bit before unmapping could
> >    allow the guest to access RAM or other MMIO regions that are
> >    exposed once the BARs are no longer mapped?
> 
> Disable before unmap is what we want for safety, so I'm not sure
> if you've simply mis-typed your reply. And disabling early is also in
> line with your desire of doing the cmd register writes first.

I agree, although I don't see the safety side of this. Disabling
before removing the p2m mappings gives the domain a window where it
can access whatever the BAR was mapped over, maybe a BAR from a
different device? In any case this won't work correctly to start
with.

> 
> >> In the interest of later enabling of the code for DomU, any
> >> such discussion should, as far as possible, avoid argumentation along
> >> the Dom0-only line.
> > 
> > My plan is that DomUs won't be allowed to toggle the memory decoding
> > bit, and it's going to be always enabled, like it's currently done for
> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
> > going to trigger a change to the p2m (map or unmap) but the command
> > register will always have the memory decoding bit enabled.
> 
> But this isn't entirely correct, even if we've got away with this
> so far. But we're mostly considering well-behaved guests and
> devices. What if one actually triggers bus activity in parallel to
> a BAR change?

Well, that's likely to not work properly in any case with or without
disabling the memory decoding bit?

But I don't see how that's going to affect Xen stability (or what the
domain is attempting to achieve with it).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
> On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
>> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
>> > My plan is that DomUs won't be allowed to toggle the memory decoding
>> > bit, and it's going to be always enabled, like it's currently done for
>> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
>> > going to trigger a change to the p2m (map or unmap) but the command
>> > register will always have the memory decoding bit enabled.
>> 
>> But this isn't entirely correct, even if we've got away with this
>> so far. But we're mostly considering well-behaved guests and
>> devices. What if one actually triggers bus activity in parallel to
>> a BAR change?
> 
> Well, that's likely to not work properly in any case with or without
> disabling the memory decoding bit?

Of course not.

> But I don't see how that's going to affect Xen stability (or what the
> domain is attempting to achieve with it).

"I don't see how ..." != "That's not going to ...". And in case my
prior way of wording it was ambiguous: We very much need to
think about malicious guests (once any of this is to be extended
to DomU-s). Hence a goal of "I want to crash Xen" needs to be
taken into consideration.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

On Wed, Nov 28, 2018 at 10:04:33AM -0700, Jan Beulich wrote:
> >>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
> > On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
> >> > My plan is that DomUs won't be allowed to toggle the memory decoding
> >> > bit, and it's going to be always enabled, like it's currently done for
> >> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
> >> > going to trigger a change to the p2m (map or unmap) but the command
> >> > register will always have the memory decoding bit enabled.
> >> 
> >> But this isn't entirely correct, even if we've got away with this
> >> so far. But we're mostly considering well-behaved guests and
> >> devices. What if one actually triggers bus activity in parallel to
> >> a BAR change?
> > 
> > Well, that's likely to not work properly in any case with or without
> > disabling the memory decoding bit?
> 
> Of course not.
> 
> > But I don't see how that's going to affect Xen stability (or what the
> > domain is attempting to achieve with it).
> 
> "I don't see how ..." != "That's not going to ...". And in case my
> prior way of wording it was ambiguous: We very much need to
> think about malicious guests (once any of this is to be extended
> to DomU-s). Hence a goal of "I want to crash Xen" needs to be
> taken into consideration.

Right, so Xen might what to disable memory decoding for DomUs also.

But that's orthogonal to whether we agree that the write to the
command register can happen before the p2m modifications, both in the
map and the unmap case. I think so, but I would like to be sure you
agree before I code this.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 28.11.18 at 18:48, <roger.pau@citrix.com> wrote:
> On Wed, Nov 28, 2018 at 10:04:33AM -0700, Jan Beulich wrote:
>> >>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
>> > On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
>> >> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
>> >> > My plan is that DomUs won't be allowed to toggle the memory decoding
>> >> > bit, and it's going to be always enabled, like it's currently done for
>> >> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
>> >> > going to trigger a change to the p2m (map or unmap) but the command
>> >> > register will always have the memory decoding bit enabled.
>> >> 
>> >> But this isn't entirely correct, even if we've got away with this
>> >> so far. But we're mostly considering well-behaved guests and
>> >> devices. What if one actually triggers bus activity in parallel to
>> >> a BAR change?
>> > 
>> > Well, that's likely to not work properly in any case with or without
>> > disabling the memory decoding bit?
>> 
>> Of course not.
>> 
>> > But I don't see how that's going to affect Xen stability (or what the
>> > domain is attempting to achieve with it).
>> 
>> "I don't see how ..." != "That's not going to ...". And in case my
>> prior way of wording it was ambiguous: We very much need to
>> think about malicious guests (once any of this is to be extended
>> to DomU-s). Hence a goal of "I want to crash Xen" needs to be
>> taken into consideration.
> 
> Right, so Xen might what to disable memory decoding for DomUs also.
> 
> But that's orthogonal to whether we agree that the write to the
> command register can happen before the p2m modifications, both in the
> map and the unmap case. I think so, but I would like to be sure you
> agree before I code this.

Thing is, as said before, I'm not sure. I can be convinced by, well,
convincing arguments (which a proof that nothing bad can happen
would be, but anything less likely wouldn't).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

On Thu, Nov 29, 2018 at 02:25:02AM -0700, Jan Beulich wrote:
> >>> On 28.11.18 at 18:48, <roger.pau@citrix.com> wrote:
> > On Wed, Nov 28, 2018 at 10:04:33AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
> >> > On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
> >> >> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
> >> >> > My plan is that DomUs won't be allowed to toggle the memory decoding
> >> >> > bit, and it's going to be always enabled, like it's currently done for
> >> >> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
> >> >> > going to trigger a change to the p2m (map or unmap) but the command
> >> >> > register will always have the memory decoding bit enabled.
> >> >> 
> >> >> But this isn't entirely correct, even if we've got away with this
> >> >> so far. But we're mostly considering well-behaved guests and
> >> >> devices. What if one actually triggers bus activity in parallel to
> >> >> a BAR change?
> >> > 
> >> > Well, that's likely to not work properly in any case with or without
> >> > disabling the memory decoding bit?
> >> 
> >> Of course not.
> >> 
> >> > But I don't see how that's going to affect Xen stability (or what the
> >> > domain is attempting to achieve with it).
> >> 
> >> "I don't see how ..." != "That's not going to ...". And in case my
> >> prior way of wording it was ambiguous: We very much need to
> >> think about malicious guests (once any of this is to be extended
> >> to DomU-s). Hence a goal of "I want to crash Xen" needs to be
> >> taken into consideration.
> > 
> > Right, so Xen might what to disable memory decoding for DomUs also.
> > 
> > But that's orthogonal to whether we agree that the write to the
> > command register can happen before the p2m modifications, both in the
> > map and the unmap case. I think so, but I would like to be sure you
> > agree before I code this.
> 
> Thing is, as said before, I'm not sure. I can be convinced by, well,
> convincing arguments (which a proof that nothing bad can happen
> would be, but anything less likely wouldn't).

Hm, OK, please bear with me because I'm afraid I will need some help
in order to provide such argument.

We agree that the end result is going to be the same, ie: a p2m change
and a write to the device command register. Then the issue is the
order in which to perform those, and whether that order will have a
security impact.

We also agree that in the unmap case it's best to first disable memory
decoding and then perform the p2m unmap. So the only remaining
problematic case is the map action.

I guess the only vector of a possible attack could be other vCPUs in a
SMP guest, that could poke at either the device registers or the p2m
after the memory decoding bit has been set and while the map operation
is taking place:

 - Writing to the BARs MMIO while performing the p2m map and with the
   memory decoding bit is enabled can result in missing writes not
   reaching the device, because the p2m entries are not yet setup.
   This can also be triggered by the guest when the BARs are
   completely mapped by performing incorrect writes.

 - Attempting to program a DMA transaction to the device MMIO BAR
   regions will result in IOMMU page faults, the same can be achieved by
   attempting to perform DMA transactions to unpopulated memory
   regions.

I think there are other scenarios you are worried about and I'm
missing, could you please point them out?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 29.11.18 at 11:25, <roger.pau@citrix.com> wrote:
> On Thu, Nov 29, 2018 at 02:25:02AM -0700, Jan Beulich wrote:
>> >>> On 28.11.18 at 18:48, <roger.pau@citrix.com> wrote:
>> > On Wed, Nov 28, 2018 at 10:04:33AM -0700, Jan Beulich wrote:
>> >> >>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
>> >> > On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
>> >> >> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
>> >> >> > My plan is that DomUs won't be allowed to toggle the memory decoding
>> >> >> > bit, and it's going to be always enabled, like it's currently done for
>> >> >> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
>> >> >> > going to trigger a change to the p2m (map or unmap) but the command
>> >> >> > register will always have the memory decoding bit enabled.
>> >> >> 
>> >> >> But this isn't entirely correct, even if we've got away with this
>> >> >> so far. But we're mostly considering well-behaved guests and
>> >> >> devices. What if one actually triggers bus activity in parallel to
>> >> >> a BAR change?
>> >> > 
>> >> > Well, that's likely to not work properly in any case with or without
>> >> > disabling the memory decoding bit?
>> >> 
>> >> Of course not.
>> >> 
>> >> > But I don't see how that's going to affect Xen stability (or what the
>> >> > domain is attempting to achieve with it).
>> >> 
>> >> "I don't see how ..." != "That's not going to ...". And in case my
>> >> prior way of wording it was ambiguous: We very much need to
>> >> think about malicious guests (once any of this is to be extended
>> >> to DomU-s). Hence a goal of "I want to crash Xen" needs to be
>> >> taken into consideration.
>> > 
>> > Right, so Xen might what to disable memory decoding for DomUs also.
>> > 
>> > But that's orthogonal to whether we agree that the write to the
>> > command register can happen before the p2m modifications, both in the
>> > map and the unmap case. I think so, but I would like to be sure you
>> > agree before I code this.
>> 
>> Thing is, as said before, I'm not sure. I can be convinced by, well,
>> convincing arguments (which a proof that nothing bad can happen
>> would be, but anything less likely wouldn't).
> 
> Hm, OK, please bear with me because I'm afraid I will need some help
> in order to provide such argument.
> 
> We agree that the end result is going to be the same, ie: a p2m change
> and a write to the device command register. Then the issue is the
> order in which to perform those, and whether that order will have a
> security impact.
> 
> We also agree that in the unmap case it's best to first disable memory
> decoding and then perform the p2m unmap. So the only remaining
> problematic case is the map action.
> 
> I guess the only vector of a possible attack could be other vCPUs in a
> SMP guest, that could poke at either the device registers or the p2m
> after the memory decoding bit has been set and while the map operation
> is taking place:
> 
>  - Writing to the BARs MMIO while performing the p2m map and with the
>    memory decoding bit is enabled can result in missing writes not
>    reaching the device, because the p2m entries are not yet setup.
>    This can also be triggered by the guest when the BARs are
>    completely mapped by performing incorrect writes.

Not sure what "incorrect writes" you mean here. Ones to other GFNs
are not targeted at the device in question anyway. But yes, P2M
mappings not in place _should_ not have any bad effects. My issue
is with the "should not" here, which I'd like to be "cannot".

>  - Attempting to program a DMA transaction to the device MMIO BAR
>    regions will result in IOMMU page faults, the same can be achieved by
>    attempting to perform DMA transactions to unpopulated memory
>    regions.
> 
> I think there are other scenarios you are worried about and I'm
> missing, could you please point them out?

That's the point: I can't point out other scenarios, but I also can't
convince myself that there are none. What I'm concerned about
from a more abstract pov are any actions by the guest which might
ultimately lead to master or target aborts (or alike), which may in
turn cause #SERR to be signaled. Yet then again we all know that
PCI pass-through is not perfectly secure despite all claims (or
should I say wishes), so maybe all of this is really tolerable.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Roger Pau Monné]

On Thu, Nov 29, 2018 at 03:41:07AM -0700, Jan Beulich wrote:
> >>> On 29.11.18 at 11:25, <roger.pau@citrix.com> wrote:
> > On Thu, Nov 29, 2018 at 02:25:02AM -0700, Jan Beulich wrote:
> >> >>> On 28.11.18 at 18:48, <roger.pau@citrix.com> wrote:
> >> > On Wed, Nov 28, 2018 at 10:04:33AM -0700, Jan Beulich wrote:
> >> >> >>> On 28.11.18 at 17:54, <roger.pau@citrix.com> wrote:
> >> >> > On Wed, Nov 28, 2018 at 09:22:16AM -0700, Jan Beulich wrote:
> >> >> >> >>> On 28.11.18 at 16:41, <roger.pau@citrix.com> wrote:
> >> >> >> > My plan is that DomUs won't be allowed to toggle the memory decoding
> >> >> >> > bit, and it's going to be always enabled, like it's currently done for
> >> >> >> > pci-passthrough in QEMU. Toggling the memory decoding bit in a DomU is
> >> >> >> > going to trigger a change to the p2m (map or unmap) but the command
> >> >> >> > register will always have the memory decoding bit enabled.
> >> >> >> 
> >> >> >> But this isn't entirely correct, even if we've got away with this
> >> >> >> so far. But we're mostly considering well-behaved guests and
> >> >> >> devices. What if one actually triggers bus activity in parallel to
> >> >> >> a BAR change?
> >> >> > 
> >> >> > Well, that's likely to not work properly in any case with or without
> >> >> > disabling the memory decoding bit?
> >> >> 
> >> >> Of course not.
> >> >> 
> >> >> > But I don't see how that's going to affect Xen stability (or what the
> >> >> > domain is attempting to achieve with it).
> >> >> 
> >> >> "I don't see how ..." != "That's not going to ...". And in case my
> >> >> prior way of wording it was ambiguous: We very much need to
> >> >> think about malicious guests (once any of this is to be extended
> >> >> to DomU-s). Hence a goal of "I want to crash Xen" needs to be
> >> >> taken into consideration.
> >> > 
> >> > Right, so Xen might what to disable memory decoding for DomUs also.
> >> > 
> >> > But that's orthogonal to whether we agree that the write to the
> >> > command register can happen before the p2m modifications, both in the
> >> > map and the unmap case. I think so, but I would like to be sure you
> >> > agree before I code this.
> >> 
> >> Thing is, as said before, I'm not sure. I can be convinced by, well,
> >> convincing arguments (which a proof that nothing bad can happen
> >> would be, but anything less likely wouldn't).
> > 
> > Hm, OK, please bear with me because I'm afraid I will need some help
> > in order to provide such argument.
> > 
> > We agree that the end result is going to be the same, ie: a p2m change
> > and a write to the device command register. Then the issue is the
> > order in which to perform those, and whether that order will have a
> > security impact.
> > 
> > We also agree that in the unmap case it's best to first disable memory
> > decoding and then perform the p2m unmap. So the only remaining
> > problematic case is the map action.
> > 
> > I guess the only vector of a possible attack could be other vCPUs in a
> > SMP guest, that could poke at either the device registers or the p2m
> > after the memory decoding bit has been set and while the map operation
> > is taking place:
> > 
> >  - Writing to the BARs MMIO while performing the p2m map and with the
> >    memory decoding bit is enabled can result in missing writes not
> >    reaching the device, because the p2m entries are not yet setup.
> >    This can also be triggered by the guest when the BARs are
> >    completely mapped by performing incorrect writes.
> 
> Not sure what "incorrect writes" you mean here. Ones to other GFNs
> are not targeted at the device in question anyway. But yes, P2M
> mappings not in place _should_ not have any bad effects. My issue
> is with the "should not" here, which I'd like to be "cannot".
> 
> >  - Attempting to program a DMA transaction to the device MMIO BAR
> >    regions will result in IOMMU page faults, the same can be achieved by
> >    attempting to perform DMA transactions to unpopulated memory
> >    regions.
> > 
> > I think there are other scenarios you are worried about and I'm
> > missing, could you please point them out?
> 
> That's the point: I can't point out other scenarios, but I also can't
> convince myself that there are none. What I'm concerned about
> from a more abstract pov are any actions by the guest which might
> ultimately lead to master or target aborts (or alike), which may in
> turn cause #SERR to be signaled.

I cannot see how interactions with a device with half-mapped BARs
could trigger aborts that cannot be triggered when the device has
fully mapped BARs. Ie: if there's indeed a way to trigger such aborts
it would also be possible to do so with fully mapped BARs.

> Yet then again we all know that
> PCI pass-through is not perfectly secure despite all claims (or
> should I say wishes), so maybe all of this is really tolerable.

Well, we already know there are devices that expose backdoors to the
PCI config space in a BAR, so it's mostly a question of whether you
trust the devices you are passing through.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: vpci: deferral of register write until p2m changes are done

[Jan Beulich]

>>> On 29.11.18 at 14:44, <roger.pau@citrix.com> wrote:
> I cannot see how interactions with a device with half-mapped BARs
> could trigger aborts that cannot be triggered when the device has
> fully mapped BARs. Ie: if there's indeed a way to trigger such aborts
> it would also be possible to do so with fully mapped BARs.

Whether comparing with the fully mapped case or the fully
unmapped one is more relevant I can't tell. In any event I
don't think we're going to get anywhere here without either
someone chiming in who has better PCIe knowledge than
me, or without you submitting a patch with a sufficiently
convincing description.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel