From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] lock down hypercall continuation encoding masks Date: Fri, 5 Dec 2014 14:36:18 +0000 Message-ID: <5481C2E2.1060306@citrix.com> References: <5481A589020000780004D1E3@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Xwtzb-0005qB-PZ for xen-devel@lists.xenproject.org; Fri, 05 Dec 2014 14:36:23 +0000 In-Reply-To: <5481A589020000780004D1E3@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , xen-devel Cc: Ian Campbell , Keir Fraser , Tim Deegan , Ian Jackson List-Id: xen-devel@lists.xenproject.org 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 > Signed-off-by: Jan Beulich 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