[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] lock down hypercall continuation encoding masks
On 05/12/14 11:31, Jan Beulich wrote: > Andrew validly points out that even if these masks aren't a formal part > of the hypercall interface, we aren't free to change them: A guest > suspended for migration in the middle of a continuation would fail to > work if resumed on a hypervisor using a different value. Hence add > respective comments to their definitions. > > Additionally, to help future extensibility as well as in the spirit of > reducing undefined behavior as much as possible, refuse hypercalls made > with the respective bits non-zero when the respective sub-ops don't > make use of those bits. > > Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx> General principle looks good. A couple of issues. > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -4661,9 +4661,8 @@ int xenmem_add_to_physmap_one( > long arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > int rc; > - int op = cmd & MEMOP_CMD_MASK; This needs a blanket start_iter check, as do_memory_op() has not done so. The ARM code also needs one, as the caller has applied partial checks. > > - switch ( op ) > + switch ( cmd ) > { > case XENMEM_set_memory_map: > { > --- a/xen/arch/x86/x86_64/mm.c > +++ b/xen/arch/x86/x86_64/mm.c > @@ -906,9 +906,8 @@ long subarch_memory_op(unsigned long cmd > xen_pfn_t mfn, last_mfn; > unsigned int i; > long rc = 0; > - int op = cmd & MEMOP_CMD_MASK; It is probably best to have a blanket check here even if arch_memory_op() has a check. It will reduce the chance of the check being missed if/when arch_memory_op() gains a presentable subop. > > - switch ( op ) > + switch ( cmd ) > { > case XENMEM_machphys_mfn_list: > if ( copy_from_guest(&xmml, arg, 1) ) > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -977,6 +992,9 @@ long do_memory_op(unsigned long cmd, XEN > unsigned int dom_vnodes, dom_vranges, dom_vcpus; > struct vnuma_info tmp; > > + if ( unlikely(start_extent) ) > + return -ENOSYS; > + > /* > * Guest passes nr_vnodes, number of regions and nr_vcpus thus > * we know how much memory guest has allocated. XENMEM_get_vnumainfo needs a guard. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |