[v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tiejun Chen]

Currently we don't allow passing through any group devices which are
sharing same RMRR entry since it would break security among VMs. And
indeed, we expect we can figure out a better way to handle this kind
of case completely.

But before the group assignment gets implemented, we might make this
permission dependent on our RMRR policy. So, now it would be allowed
in the relaxed mode.

CC: Yang Zhang <yang.z.zhang@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
---
v2:

* Sync code comments
* refactor vaiable "relaxed" as bool_t
* s/XENLOG_G_ERR VTDPREFIX/XENLOG_G_WARNING VTDPREFIX
* Try to refine print message

 xen/drivers/passthrough/vtd/iommu.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 836aed5..7b45bff 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -2297,7 +2297,9 @@ static int intel_iommu_assign_device(
     /*
      * In rare cases one given rmrr is shared by multiple devices but
      * obviously this would put the security of a system at risk. So
-     * we should prevent from this sort of device assignment.
+     * we would prevent from this sort of device assignment. But this
+     * can be permitted if user set
+     *      "pci = [ 'sbdf, rdm_policy=relaxed' ]"
      *
      * TODO: in the future we can introduce group device assignment
      * interface to make sure devices sharing RMRR are assigned to the
@@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
              PCI_DEVFN2(bdf) == devfn &&
              rmrr->scope.devices_cnt > 1 )
         {
-            printk(XENLOG_G_ERR VTDPREFIX
-                   " cannot assign %04x:%02x:%02x.%u"
+            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
+
+            printk(XENLOG_G_WARNING VTDPREFIX
+                   " It's %s to assign %04x:%02x:%02x.%u"
                    " with shared RMRR at %"PRIx64" for Dom%d.\n",
+                   relaxed ? "risky" : "disallowed",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                    rmrr->base_address, d->domain_id);
-            return -EPERM;
+            if ( !relaxed )
+                return -EPERM;
         }
     }
 
-- 
1.9.1

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
> @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
>               PCI_DEVFN2(bdf) == devfn &&
>               rmrr->scope.devices_cnt > 1 )
>          {
> -            printk(XENLOG_G_ERR VTDPREFIX
> -                   " cannot assign %04x:%02x:%02x.%u"
> +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
> +
> +            printk(XENLOG_G_WARNING VTDPREFIX

Well, I can live with this always being a warning, but it's not what I
had asked for. The VT-d maintainers will have to judge.

Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

On 9/9/2015 2:54 PM, Jan Beulich wrote:
>>>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
>> @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
>>               PCI_DEVFN2(bdf) == devfn &&
>>               rmrr->scope.devices_cnt > 1 )
>>          {
>> -            printk(XENLOG_G_ERR VTDPREFIX
>> -                   " cannot assign %04x:%02x:%02x.%u"
>> +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
>> +
>> +            printk(XENLOG_G_WARNING VTDPREFIX
>
> Well, I can live with this always being a warning, but it's not what I
> had asked for. The VT-d maintainers will have to judge.
>

Kevin,

Any comments?

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, September 09, 2015 2:55 PM
> 
> >>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
> >               PCI_DEVFN2(bdf) == devfn &&
> >               rmrr->scope.devices_cnt > 1 )
> >          {
> > -            printk(XENLOG_G_ERR VTDPREFIX
> > -                   " cannot assign %04x:%02x:%02x.%u"
> > +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
> > +
> > +            printk(XENLOG_G_WARNING VTDPREFIX
> 
> Well, I can live with this always being a warning, but it's not what I
> had asked for. The VT-d maintainers will have to judge.
> 

Need to have separate warning/error level for relax/strict.

However I don't think this patch is a right fix. So far relax/strict policy
is per-domain. what about one VM specifies relax while another VM
specifies strict when each is assigned with a device sharing rmrr
with the other? In that case it becomes a system-wide security hole.

Once we add code to track group relationship cross domains, it'd be
close to the final fix to support group assignment which originally target 
4.7. It might be risky to add that in 4.6.

So my suggestion is to live with current limitation.

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

> Need to have separate warning/error level for relax/strict.
>
> However I don't think this patch is a right fix. So far relax/strict policy
> is per-domain. what about one VM specifies relax while another VM
> specifies strict when each is assigned with a device sharing rmrr
> with the other? In that case it becomes a system-wide security hole.
>
> Once we add code to track group relationship cross domains, it'd be
> close to the final fix to support group assignment which originally target
> 4.7. It might be risky to add that in 4.6.

Yes.

>
> So my suggestion is to live with current limitation.
>

But recently someone was encountering this problem.

http://www.gossamer-threads.com/lists/xen/devel/391684?page=last

We'd better figure out a simple way to this regression.

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Thursday, September 10, 2015 1:47 PM
> 
> > Need to have separate warning/error level for relax/strict.
> >
> > However I don't think this patch is a right fix. So far relax/strict policy
> > is per-domain. what about one VM specifies relax while another VM
> > specifies strict when each is assigned with a device sharing rmrr
> > with the other? In that case it becomes a system-wide security hole.
> >
> > Once we add code to track group relationship cross domains, it'd be
> > close to the final fix to support group assignment which originally target
> > 4.7. It might be risky to add that in 4.6.
> 
> Yes.
> 
> >
> > So my suggestion is to live with current limitation.
> >
> 
> But recently someone was encountering this problem.
> 
> http://www.gossamer-threads.com/lists/xen/devel/391684?page=last
> 
> We'd better figure out a simple way to this regression.
> 

I'm not sure how popular that motherboard is used... To me security is
important so having some limitation for this purpose is acceptable. But
I'd also like to hear comments from Jan and Wei. If they think regression
is more important (anyway we're not causing new security problem, it's
there before), I'm OK with this patch (besides you need fix print level)

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 10.09.15 at 07:23, <kevin.tian@intel.com> wrote:
>>  From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Wednesday, September 09, 2015 2:55 PM
>> 
>> >>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
>> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
>> >               PCI_DEVFN2(bdf) == devfn &&
>> >               rmrr->scope.devices_cnt > 1 )
>> >          {
>> > -            printk(XENLOG_G_ERR VTDPREFIX
>> > -                   " cannot assign %04x:%02x:%02x.%u"
>> > +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
>> > +
>> > +            printk(XENLOG_G_WARNING VTDPREFIX
>> 
>> Well, I can live with this always being a warning, but it's not what I
>> had asked for. The VT-d maintainers will have to judge.
>> 
> 
> Need to have separate warning/error level for relax/strict.
> 
> However I don't think this patch is a right fix. So far relax/strict policy
> is per-domain. what about one VM specifies relax while another VM
> specifies strict when each is assigned with a device sharing rmrr
> with the other? In that case it becomes a system-wide security hole.

The one specifying "strict" won't gets its device assigned (due to
the code above, taking the path that was there already without
the patch), so I don't see the security issue.

Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Wei Liu]

On Thu, Sep 10, 2015 at 02:09:39AM -0600, Jan Beulich wrote:
> >>> On 10.09.15 at 07:23, <kevin.tian@intel.com> wrote:
> >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Wednesday, September 09, 2015 2:55 PM
> >> 
> >> >>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
> >> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
> >> >               PCI_DEVFN2(bdf) == devfn &&
> >> >               rmrr->scope.devices_cnt > 1 )
> >> >          {
> >> > -            printk(XENLOG_G_ERR VTDPREFIX
> >> > -                   " cannot assign %04x:%02x:%02x.%u"
> >> > +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
> >> > +
> >> > +            printk(XENLOG_G_WARNING VTDPREFIX
> >> 
> >> Well, I can live with this always being a warning, but it's not what I
> >> had asked for. The VT-d maintainers will have to judge.
> >> 
> > 
> > Need to have separate warning/error level for relax/strict.
> > 
> > However I don't think this patch is a right fix. So far relax/strict policy
> > is per-domain. what about one VM specifies relax while another VM
> > specifies strict when each is assigned with a device sharing rmrr
> > with the other? In that case it becomes a system-wide security hole.
> 
> The one specifying "strict" won't gets its device assigned (due to
> the code above, taking the path that was there already without
> the patch), so I don't see the security issue.
> 

Agreed. A VM can't get such device assigned in the first place, so the
hypothetical scenario doesn't exist.

Wei.

> Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Håkon Alstadheim]

Den 10. sep. 2015 08:06, skrev Tian, Kevin:
>> From: Chen, Tiejun
>> Sent: Thursday, September 10, 2015 1:47 PM
>>
>> But recently someone was encountering this problem.
>>
>> http://www.gossamer-threads.com/lists/xen/devel/391684?page=last
>>
>> We'd better figure out a simple way to this regression.
>>
> I'm not sure how popular that motherboard is used...
I've got the same devices on Asus Z10PE-D8 WS. Probably used
by several motherboard-manufacturers.


>  To me security is
> important so having some limitation for this purpose is acceptable.
PCIE add-in cards for USB are hard to work with, see [1],  so maximum use
of the on-board USB is a high priority for me, and I'd think others aswell.
>  But
> I'd also like to hear comments from Jan and Wei. If they think regression
> is more important (anyway we're not causing new security problem, it's
> there before), I'm OK with this patch (besides you need fix print level)
>
>
[1]
<http://www.gossamer-threads.com/lists/gentoo/user/305889?search_string=USB%203.0;#305889>

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Håkon Alstadheim]

Den 10. sep. 2015 13:04, skrev Håkon Alstadheim:
> Den 10. sep. 2015 08:06, skrev Tian, Kevin:
>>> From: Chen, Tiejun
>>> Sent: Thursday, September 10, 2015 1:47 PM
>>>
>>> But recently someone was encountering this problem.
>>>
>>> http://www.gossamer-threads.com/lists/xen/devel/391684?page=last
>>>
>>> We'd better figure out a simple way to this regression.
>>>
>> I'm not sure how popular that motherboard is used...
> I've got the same devices on Asus Z10PE-D8 WS. Probably used
> by several motherboard-manufacturers.

To be very precice: I've got _similar_ devices in my motherboard chipset.
Thus we know Intel chipset C200 has this issue, and I am fairly certain
that C610/X99 also has this issue.

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: Thursday, September 10, 2015 6:37 PM
> 
> On Thu, Sep 10, 2015 at 02:09:39AM -0600, Jan Beulich wrote:
> > >>> On 10.09.15 at 07:23, <kevin.tian@intel.com> wrote:
> > >>  From: Jan Beulich [mailto:JBeulich@suse.com]
> > >> Sent: Wednesday, September 09, 2015 2:55 PM
> > >>
> > >> >>> On 09.09.15 at 03:59, <tiejun.chen@intel.com> wrote:
> > >> > @@ -2310,12 +2312,16 @@ static int intel_iommu_assign_device(
> > >> >               PCI_DEVFN2(bdf) == devfn &&
> > >> >               rmrr->scope.devices_cnt > 1 )
> > >> >          {
> > >> > -            printk(XENLOG_G_ERR VTDPREFIX
> > >> > -                   " cannot assign %04x:%02x:%02x.%u"
> > >> > +            bool_t relaxed = !!(flag & XEN_DOMCTL_DEV_RDM_RELAXED);
> > >> > +
> > >> > +            printk(XENLOG_G_WARNING VTDPREFIX
> > >>
> > >> Well, I can live with this always being a warning, but it's not what I
> > >> had asked for. The VT-d maintainers will have to judge.
> > >>
> > >
> > > Need to have separate warning/error level for relax/strict.
> > >
> > > However I don't think this patch is a right fix. So far relax/strict policy
> > > is per-domain. what about one VM specifies relax while another VM
> > > specifies strict when each is assigned with a device sharing rmrr
> > > with the other? In that case it becomes a system-wide security hole.
> >
> > The one specifying "strict" won't gets its device assigned (due to
> > the code above, taking the path that was there already without
> > the patch), so I don't see the security issue.
> >
> 
> Agreed. A VM can't get such device assigned in the first place, so the
> hypothetical scenario doesn't exist.
> 

Sorry it's a bad example. My actual concern is that we can't count
on this per-VM relax/strict policy to prevent group devices assigned
to different VM. In that case it's definitely a security hole since
one VM may clobber shared RMRR to impact another VM. So right
example for that scenario is both VMs specified with 'relax'. 

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

>> > > Need to have separate warning/error level for relax/strict.
>> > >
>> > > However I don't think this patch is a right fix. So far relax/strict policy
>> > > is per-domain. what about one VM specifies relax while another VM
>> > > specifies strict when each is assigned with a device sharing rmrr
>> > > with the other? In that case it becomes a system-wide security hole.
>> >
>> > The one specifying "strict" won't gets its device assigned (due to
>> > the code above, taking the path that was there already without
>> > the patch), so I don't see the security issue.
>> >
>>
>> Agreed. A VM can't get such device assigned in the first place, so the
>> hypothetical scenario doesn't exist.
>>
>
> Sorry it's a bad example. My actual concern is that we can't count
> on this per-VM relax/strict policy to prevent group devices assigned
> to different VM. In that case it's definitely a security hole since
> one VM may clobber shared RMRR to impact another VM. So right
> example for that scenario is both VMs specified with 'relax'.

What if one of group devices is still owned by Dom0?

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Friday, September 11, 2015 8:56 AM
> 
> >> > > Need to have separate warning/error level for relax/strict.
> >> > >
> >> > > However I don't think this patch is a right fix. So far relax/strict policy
> >> > > is per-domain. what about one VM specifies relax while another VM
> >> > > specifies strict when each is assigned with a device sharing rmrr
> >> > > with the other? In that case it becomes a system-wide security hole.
> >> >
> >> > The one specifying "strict" won't gets its device assigned (due to
> >> > the code above, taking the path that was there already without
> >> > the patch), so I don't see the security issue.
> >> >
> >>
> >> Agreed. A VM can't get such device assigned in the first place, so the
> >> hypothetical scenario doesn't exist.
> >>
> >
> > Sorry it's a bad example. My actual concern is that we can't count
> > on this per-VM relax/strict policy to prevent group devices assigned
> > to different VM. In that case it's definitely a security hole since
> > one VM may clobber shared RMRR to impact another VM. So right
> > example for that scenario is both VMs specified with 'relax'.
> 
> What if one of group devices is still owned by Dom0?
> 

It's also risky since other VM may attack Dom0 in such scenario.

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

>> >> > > However I don't think this patch is a right fix. So far relax/strict policy
>> >> > > is per-domain. what about one VM specifies relax while another VM
>> >> > > specifies strict when each is assigned with a device sharing rmrr
>> >> > > with the other? In that case it becomes a system-wide security hole.
>> >> >
>> >> > The one specifying "strict" won't gets its device assigned (due to
>> >> > the code above, taking the path that was there already without
>> >> > the patch), so I don't see the security issue.
>> >> >
>> >>
>> >> Agreed. A VM can't get such device assigned in the first place, so the
>> >> hypothetical scenario doesn't exist.
>> >>
>> >
>> > Sorry it's a bad example. My actual concern is that we can't count
>> > on this per-VM relax/strict policy to prevent group devices assigned
>> > to different VM. In that case it's definitely a security hole since
>> > one VM may clobber shared RMRR to impact another VM. So right
>> > example for that scenario is both VMs specified with 'relax'.
>>
>> What if one of group devices is still owned by Dom0?
>>
>
> It's also risky since other VM may attack Dom0 in such scenario.
>

In my opinion, Dom0 should have a big impact...

Anyway, this always means we have to start refactoring some codes. For 
example, we are probably going to introduce some new fields in struct 
acpi_rmrr_unit, just like,

int domain_id -> Distinguish which domain owns this unit
unsigned int flag -> Record state: XEN_DOMCTL_DEV_RDM_RELAXE or 
!XEN_DOMCTL_DEV_RDM_RELAXE

This should involve several sections, such as parsing rmrr, setup 
hwdomain and assign/remove device.

But I'm not sure if this is good to handle current problem. Actually I 
prefer to work on current patch just now, and then we can start 
discussing our final solution :)

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Friday, September 11, 2015 10:21 AM
> 
> >> >> > > However I don't think this patch is a right fix. So far relax/strict policy
> >> >> > > is per-domain. what about one VM specifies relax while another VM
> >> >> > > specifies strict when each is assigned with a device sharing rmrr
> >> >> > > with the other? In that case it becomes a system-wide security hole.
> >> >> >
> >> >> > The one specifying "strict" won't gets its device assigned (due to
> >> >> > the code above, taking the path that was there already without
> >> >> > the patch), so I don't see the security issue.
> >> >> >
> >> >>
> >> >> Agreed. A VM can't get such device assigned in the first place, so the
> >> >> hypothetical scenario doesn't exist.
> >> >>
> >> >
> >> > Sorry it's a bad example. My actual concern is that we can't count
> >> > on this per-VM relax/strict policy to prevent group devices assigned
> >> > to different VM. In that case it's definitely a security hole since
> >> > one VM may clobber shared RMRR to impact another VM. So right
> >> > example for that scenario is both VMs specified with 'relax'.
> >>
> >> What if one of group devices is still owned by Dom0?
> >>
> >
> > It's also risky since other VM may attack Dom0 in such scenario.
> >
> 
> In my opinion, Dom0 should have a big impact...
> 
> Anyway, this always means we have to start refactoring some codes. For
> example, we are probably going to introduce some new fields in struct
> acpi_rmrr_unit, just like,
> 
> int domain_id -> Distinguish which domain owns this unit
> unsigned int flag -> Record state: XEN_DOMCTL_DEV_RDM_RELAXE or
> !XEN_DOMCTL_DEV_RDM_RELAXE
> 
> This should involve several sections, such as parsing rmrr, setup
> hwdomain and assign/remove device.
> 
> But I'm not sure if this is good to handle current problem. Actually I
> prefer to work on current patch just now, and then we can start
> discussing our final solution :)
> 

I think we are synced on final solution, that we must ensure devices
sharing RMRR are assigned together. Before the final solution is ready,
our previous agreement is to disallow such assignment to avoid 
security risk, which however caused one regression as reported here.
Now the question is whether we want to remove the security check
(as your patch) to solve the regression, or keep the security check 
which fix a security hole but causing some regression.

Also we should note that reusing same per-domain relax/strict flag
for such purpose is also risky. What about an user unaware of
the fact of devices sharing RMRR, and then he assigns devices to
different VM both w/ relax policy specified? It may expose attack
chance inadvertently. So if we really want to have a temporary
relax mode for group devices, I prefer to have a new parameter
so the user can have full awareness and control on it.

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 11.09.15 at 01:22, <kevin.tian@intel.com> wrote:
> Sorry it's a bad example. My actual concern is that we can't count
> on this per-VM relax/strict policy to prevent group devices assigned
> to different VM. In that case it's definitely a security hole since
> one VM may clobber shared RMRR to impact another VM. So right
> example for that scenario is both VMs specified with 'relax'. 

Sorry, no, the idea of "relax" is to allow the admin to state "I have
no security concerns". Hence we'd have a security issue only if the
default was "relax" (which iiuc it isn't, or if it were _that's_ what
would need to be alongside the presented change). Whether that
statement of the admin is because of
- knowing that the RMRR won't be used post-boot
- group-assigning the devices manually
- simply not caring (i.e. trusting the guests)
is not our business.

IOW, provided there's no way for "relax" to become the default
(Tiejun - please confirm), the patch as is should be fine.

Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, September 11, 2015 4:56 PM
> 
> >>> On 11.09.15 at 01:22, <kevin.tian@intel.com> wrote:
> > Sorry it's a bad example. My actual concern is that we can't count
> > on this per-VM relax/strict policy to prevent group devices assigned
> > to different VM. In that case it's definitely a security hole since
> > one VM may clobber shared RMRR to impact another VM. So right
> > example for that scenario is both VMs specified with 'relax'.
> 
> Sorry, no, the idea of "relax" is to allow the admin to state "I have
> no security concerns". Hence we'd have a security issue only if the
> default was "relax" (which iiuc it isn't, or if it were _that's_ what
> would need to be alongside the presented change). Whether that
> statement of the admin is because of
> - knowing that the RMRR won't be used post-boot
> - group-assigning the devices manually
> - simply not caring (i.e. trusting the guests)
> is not our business.
> 
> IOW, provided there's no way for "relax" to become the default
> (Tiejun - please confirm), the patch as is should be fine.
> 
> Jan
> 

OK, that explanation is fine to me as long as it's made clear no
security guarantee once admin uses 'relax' for any domain. Tiejun
could you resend patch with right warning/error type?

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

> OK, that explanation is fine to me as long as it's made clear no
> security guarantee once admin uses 'relax' for any domain. Tiejun
> could you resend patch with right warning/error type?
>

Sure, but a little bit makes me confused when I'm trying to address 
this. Actually most messages are same, except for logevel, so I did this 
like,

             printk(XENLOG_G_INFO VTDPREFIX " Assign %04x:%02x:%02x.%u"
                    " with shared RMRR at %"PRIx64" for Dom%d.",
                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                    rmrr->base_address, d->domain_id);
             if ( relaxed )
                 printk(XENLOG_G_WARNING VTDPREFIX " It's really risky.");
             else
                 printk(XENLOG_G_ERR VTDPREFIX " So it's disallowed!");
             printk(XENLOG_G_INFO VTDPREFIX "\n");

But looks its not better, so any idea?

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Monday, September 14, 2015 2:25 PM
> 
> > OK, that explanation is fine to me as long as it's made clear no
> > security guarantee once admin uses 'relax' for any domain. Tiejun
> > could you resend patch with right warning/error type?
> >
> 
> Sure, but a little bit makes me confused when I'm trying to address
> this. Actually most messages are same, except for logevel, so I did this
> like,
> 
>              printk(XENLOG_G_INFO VTDPREFIX " Assign %04x:%02x:%02x.%u"
>                     " with shared RMRR at %"PRIx64" for Dom%d.",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>                     rmrr->base_address, d->domain_id);
>              if ( relaxed )
>                  printk(XENLOG_G_WARNING VTDPREFIX " It's really risky.");
>              else
>                  printk(XENLOG_G_ERR VTDPREFIX " So it's disallowed!");
>              printk(XENLOG_G_INFO VTDPREFIX "\n");
> 
> But looks its not better, so any idea?
> 

en... not good. I'm going to ack original patch instead.

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Tian, Kevin]

> From: Chen, Tiejun
> Sent: Wednesday, September 09, 2015 10:00 AM
> 
> Currently we don't allow passing through any group devices which are
> sharing same RMRR entry since it would break security among VMs. And
> indeed, we expect we can figure out a better way to handle this kind
> of case completely.
> 
> But before the group assignment gets implemented, we might make this
> permission dependent on our RMRR policy. So, now it would be allowed
> in the relaxed mode.
> 
> CC: Yang Zhang <yang.z.zhang@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

Thanks
Kevin

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Wei Liu]

On Mon, Sep 14, 2015 at 06:59:56AM +0000, Tian, Kevin wrote:
> > From: Chen, Tiejun
> > Sent: Wednesday, September 09, 2015 10:00 AM
> > 
> > Currently we don't allow passing through any group devices which are
> > sharing same RMRR entry since it would break security among VMs. And
> > indeed, we expect we can figure out a better way to handle this kind
> > of case completely.
> > 
> > But before the group assignment gets implemented, we might make this
> > permission dependent on our RMRR policy. So, now it would be allowed
> > in the relaxed mode.
> > 
> > CC: Yang Zhang <yang.z.zhang@intel.com>
> > CC: Kevin Tian <kevin.tian@intel.com>
> > CC: Jan Beulich <JBeulich@suse.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> 
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> 

As far as I can tell all concerns are addressed:

Release-acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 14.09.15 at 08:24, <tiejun.chen@intel.com> wrote:
>>  OK, that explanation is fine to me as long as it's made clear no
>> security guarantee once admin uses 'relax' for any domain. Tiejun
>> could you resend patch with right warning/error type?
>>
> 
> Sure, but a little bit makes me confused when I'm trying to address 
> this. Actually most messages are same, except for logevel, so I did this 
> like,
> 
>              printk(XENLOG_G_INFO VTDPREFIX " Assign %04x:%02x:%02x.%u"
>                     " with shared RMRR at %"PRIx64" for Dom%d.",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>                     rmrr->base_address, d->domain_id);
>              if ( relaxed )
>                  printk(XENLOG_G_WARNING VTDPREFIX " It's really risky.");
>              else
>                  printk(XENLOG_G_ERR VTDPREFIX " So it's disallowed!");
>              printk(XENLOG_G_INFO VTDPREFIX "\n");
> 
> But looks its not better, so any idea?

Did you at least make an attempt to find other examples of where
we dynamically determine the log level to be used for a message?
I would assume that if you did, you'd have come to

            printk(XENLOG_GUEST "%s" VTDPREFIX
                   " It's %s to assign %04x:%02x:%02x.%u"
                   " with shared RMRR at %"PRIx64" for Dom%d.\n",
                   relaxed ? XENLOG_WARNING : XENLOG_ERROR,
                   relaxed ? "risky" : "disallowed",
                   seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
                   rmrr->base_address, d->domain_id);

pretty naturally.

Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Chen, Tiejun]

>> But looks its not better, so any idea?
>
> Did you at least make an attempt to find other examples of where
> we dynamically determine the log level to be used for a message?
> I would assume that if you did, you'd have come to
>
>              printk(XENLOG_GUEST "%s" VTDPREFIX

I didn't know this tip on Xen side and its really good.

>                     " It's %s to assign %04x:%02x:%02x.%u"
>                     " with shared RMRR at %"PRIx64" for Dom%d.\n",
>                     relaxed ? XENLOG_WARNING : XENLOG_ERROR,
>                     relaxed ? "risky" : "disallowed",
>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>                     rmrr->base_address, d->domain_id);
>
> pretty naturally.
>

But I noticed my original patch is already merged into staging. So

Wei,

Do you think if we need a small patch to improved this? Maybe you can 
squash that if necessary.

Thanks
Tiejun

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Jan Beulich]

>>> On 15.09.15 at 03:17, <tiejun.chen@intel.com> wrote:
>> > But looks its not better, so any idea?
>>
>> Did you at least make an attempt to find other examples of where
>> we dynamically determine the log level to be used for a message?
>> I would assume that if you did, you'd have come to
>>
>>              printk(XENLOG_GUEST "%s" VTDPREFIX
> 
> I didn't know this tip on Xen side and its really good.
> 
>>                     " It's %s to assign %04x:%02x:%02x.%u"
>>                     " with shared RMRR at %"PRIx64" for Dom%d.\n",
>>                     relaxed ? XENLOG_WARNING : XENLOG_ERROR,
>>                     relaxed ? "risky" : "disallowed",
>>                     seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
>>                     rmrr->base_address, d->domain_id);
>>
>> pretty naturally.
>>
> 
> But I noticed my original patch is already merged into staging. So

I committed it since Kevin had acked it, and it was better than
nothing. I'd still like to see the log level adjustment though...

Jan

Re: [v2][PATCH] xen/vtd/iommu: permit group devices to passthrough in relaxed mode

[Wei Liu]

On Tue, Sep 15, 2015 at 09:17:07AM +0800, Chen, Tiejun wrote:
> >>But looks its not better, so any idea?
> >
> >Did you at least make an attempt to find other examples of where
> >we dynamically determine the log level to be used for a message?
> >I would assume that if you did, you'd have come to
> >
> >             printk(XENLOG_GUEST "%s" VTDPREFIX
> 
> I didn't know this tip on Xen side and its really good.
> 
> >                    " It's %s to assign %04x:%02x:%02x.%u"
> >                    " with shared RMRR at %"PRIx64" for Dom%d.\n",
> >                    relaxed ? XENLOG_WARNING : XENLOG_ERROR,
> >                    relaxed ? "risky" : "disallowed",
> >                    seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
> >                    rmrr->base_address, d->domain_id);
> >
> >pretty naturally.
> >
> 
> But I noticed my original patch is already merged into staging. So
> 
> Wei,
> 
> Do you think if we need a small patch to improved this? Maybe you can squash
> that if necessary.

Feel free to send follow-up patch to improve logging message.

Wei.

> 
> Thanks
> Tiejun