| Message ID | 1471615975-9927-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 19.08.16 at 16:12, <andrew.cooper3@citrix.com> wrote: > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -903,7 +903,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > if ( op == XENMEM_populate_physmap > && (reservation.mem_flags & XENMEMF_populate_on_demand) ) > + { > + /* Disallow populating PoD pages on oneself. */ > + if ( d == curr_d ) > + { > + rcu_unlock_domain(d); > + return start_extent; > + } > + > args.memflags |= MEMF_populate_on_demand; > + } > > if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) ) > { Considering the code continues rcu_unlock_domain(d); return start_extent; } switch ( op ) { case XENMEM_increase_reservation: increase_reservation(&args); break; case XENMEM_decrease_reservation: decrease_reservation(&args); break; default: /* XENMEM_populate_physmap */ populate_physmap(&args); break; } I think it is time to move this XENMEM_populate_physmap specific code chunk into populate_physmap(), at once eliminating the need for another not entirely trivial error exit path here. Jan
On 19/08/16 15:58, Jan Beulich wrote: >>>> On 19.08.16 at 16:12, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/common/memory.c >> +++ b/xen/common/memory.c >> @@ -903,7 +903,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> >> if ( op == XENMEM_populate_physmap >> && (reservation.mem_flags & XENMEMF_populate_on_demand) ) >> + { >> + /* Disallow populating PoD pages on oneself. */ >> + if ( d == curr_d ) >> + { >> + rcu_unlock_domain(d); >> + return start_extent; >> + } >> + >> args.memflags |= MEMF_populate_on_demand; >> + } >> >> if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) ) >> { > Considering the code continues > > rcu_unlock_domain(d); > return start_extent; > } > > switch ( op ) > { > case XENMEM_increase_reservation: > increase_reservation(&args); > break; > case XENMEM_decrease_reservation: > decrease_reservation(&args); > break; > default: /* XENMEM_populate_physmap */ > populate_physmap(&args); > break; > } > > I think it is time to move this XENMEM_populate_physmap specific > code chunk into populate_physmap(), at once eliminating the need > for another not entirely trivial error exit path here. Do you mean just my code addition, or the whole if condition? The former is easy, but the latter is hard as reservation is specifically not passed into populate_physmap(). ~Andrew
>>> On 19.08.16 at 17:02, <andrew.cooper3@citrix.com> wrote: > On 19/08/16 15:58, Jan Beulich wrote: >>>>> On 19.08.16 at 16:12, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/common/memory.c >>> +++ b/xen/common/memory.c >>> @@ -903,7 +903,16 @@ long do_memory_op(unsigned long cmd, > XEN_GUEST_HANDLE_PARAM(void) arg) >>> >>> if ( op == XENMEM_populate_physmap >>> && (reservation.mem_flags & XENMEMF_populate_on_demand) ) >>> + { >>> + /* Disallow populating PoD pages on oneself. */ >>> + if ( d == curr_d ) >>> + { >>> + rcu_unlock_domain(d); >>> + return start_extent; >>> + } >>> + >>> args.memflags |= MEMF_populate_on_demand; >>> + } >>> >>> if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) ) >>> { >> Considering the code continues >> >> rcu_unlock_domain(d); >> return start_extent; >> } >> >> switch ( op ) >> { >> case XENMEM_increase_reservation: >> increase_reservation(&args); >> break; >> case XENMEM_decrease_reservation: >> decrease_reservation(&args); >> break; >> default: /* XENMEM_populate_physmap */ >> populate_physmap(&args); >> break; >> } >> >> I think it is time to move this XENMEM_populate_physmap specific >> code chunk into populate_physmap(), at once eliminating the need >> for another not entirely trivial error exit path here. > > Do you mean just my code addition, or the whole if condition? The > former is easy, but the latter is hard as reservation is specifically > not passed into populate_physmap(). Oh, I didn't realize that - just your addition then. Jan
diff --git a/xen/common/memory.c b/xen/common/memory.c index 1ead35c..ccb9207 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -903,7 +903,16 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) if ( op == XENMEM_populate_physmap && (reservation.mem_flags & XENMEMF_populate_on_demand) ) + { + /* Disallow populating PoD pages on oneself. */ + if ( d == curr_d ) + { + rcu_unlock_domain(d); + return start_extent; + } + args.memflags |= MEMF_populate_on_demand; + } if ( xsm_memory_adjust_reservation(XSM_TARGET, curr_d, d) ) {
PoD is supposed to be entirely transparent to guest, but this interface has been left exposed for a long time. The use of PoD requires careful co-ordination by the toolstack with the XENMEM_{get,set}_pod_target hypercalls, and xenstore ballooning target. The best a guest can do without toolstack cooperation crash. Furthermore, there are combinations of features (e.g. c/s c63868ff "libxl: disallow PCI device assignment for HVM guest when PoD is enabled") which a toolstack might wish to explicitly prohibit (in this case, because the two simply don't function in combination). In such cases, the guest mustn't be able to subvert the configuration chosen by the toolstack. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: George Dunlap <george.dunlap@eu.citrix.com> --- xen/common/memory.c | 9 +++++++++ 1 file changed, 9 insertions(+)