| Message ID | 1474991845-27962-30-git-send-email-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/09/16 16:57, Roger Pau Monne wrote: > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: George Dunlap <george.dunlap@citrix.com> With one note: You might consider changing the summary to "Allow HVM hardware domains (PVHv2 dom0) to perform..." That makes it clear to someone casually scanning that you're really enabling all HVM domains to get to the next step. If we ever get rid of the hardware domain check, this might be important to people using XSM, for instance. -George
>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: > --- a/xen/arch/x86/mm/p2m.c > +++ b/xen/arch/x86/mm/p2m.c > @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, > struct domain *fdom; > > ASSERT(tdom); > - if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) ) > + if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) ) > return -EINVAL; Can PV domains make it here? If not, dropping the predicate would seem the better adjustment. Jan
On 10/10/16 15:21, Jan Beulich wrote: >>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >> --- a/xen/arch/x86/mm/p2m.c >> +++ b/xen/arch/x86/mm/p2m.c >> @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, >> struct domain *fdom; >> >> ASSERT(tdom); >> - if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) ) >> + if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) ) >> return -EINVAL; > > Can PV domains make it here? If not, dropping the predicate would > seem the better adjustment. Is there any chance that in the future PV domains might accidentally get here because of some other changes in the future? If so, leaving the predicate seems like a sensible precaution to reduce the risk of XSAs down the road, since we're doing a load of checking already anyway. ;-) At the moment, nobody's going to get past he "is_hardware_domain()" except dom0, but presumably that will change once we get driver domains implemented. -George
>>> On 10.10.16 at 16:27, <george.dunlap@citrix.com> wrote: > On 10/10/16 15:21, Jan Beulich wrote: >>>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >>> --- a/xen/arch/x86/mm/p2m.c >>> +++ b/xen/arch/x86/mm/p2m.c >>> @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, >>> struct domain *fdom; >>> >>> ASSERT(tdom); >>> - if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) ) >>> + if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) ) >>> return -EINVAL; >> >> Can PV domains make it here? If not, dropping the predicate would >> seem the better adjustment. > > Is there any chance that in the future PV domains might accidentally get > here because of some other changes in the future? If so, leaving the > predicate seems like a sensible precaution to reduce the risk of XSAs > down the road, since we're doing a load of checking already anyway. ;-) In which case I'd still prefer to extend the ASSERT() right ahead of the if(). But you're the maintainer of this code, so you know best. Jan
On 10/10/16 15:50, Jan Beulich wrote: >>>> On 10.10.16 at 16:27, <george.dunlap@citrix.com> wrote: >> On 10/10/16 15:21, Jan Beulich wrote: >>>>>> On 27.09.16 at 17:57, <roger.pau@citrix.com> wrote: >>>> --- a/xen/arch/x86/mm/p2m.c >>>> +++ b/xen/arch/x86/mm/p2m.c >>>> @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, >>>> struct domain *fdom; >>>> >>>> ASSERT(tdom); >>>> - if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) ) >>>> + if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) ) >>>> return -EINVAL; >>> >>> Can PV domains make it here? If not, dropping the predicate would >>> seem the better adjustment. >> >> Is there any chance that in the future PV domains might accidentally get >> here because of some other changes in the future? If so, leaving the >> predicate seems like a sensible precaution to reduce the risk of XSAs >> down the road, since we're doing a load of checking already anyway. ;-) > > In which case I'd still prefer to extend the ASSERT() right ahead of > the if(). But you're the maintainer of this code, so you know best. ASSERT()s and BUG_ON()s are not suitable for checks with security implications. Unless we specifically test for PV guests making this hypercall, both are very likely to slip entirely through any testing we do; in which case the former would become a full security issue, and the latter becomes a DoS. -George
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c index 44492ae..c989b60 100644 --- a/xen/arch/x86/mm/p2m.c +++ b/xen/arch/x86/mm/p2m.c @@ -2793,7 +2793,7 @@ int p2m_add_foreign(struct domain *tdom, unsigned long fgfn, struct domain *fdom; ASSERT(tdom); - if ( foreigndom == DOMID_SELF || !is_pvh_domain(tdom) ) + if ( foreigndom == DOMID_SELF || !has_hvm_container_domain(tdom) ) return -EINVAL; /* * pvh fixme: until support is added to p2m teardown code to cleanup any
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/x86/mm/p2m.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)