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 15:24:08 +0000 Message-ID: <5481CE18.9020002@citrix.com> References: <5481A589020000780004D1E3@mail.emea.novell.com> <5481C2E2.1060306@citrix.com> <5481D38E020000780004D35C@mail.emea.novell.com> <5481C8BC.4020906@citrix.com> <5481DAD4020000780004D3EA@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Xwujw-0001ZE-GP for xen-devel@lists.xenproject.org; Fri, 05 Dec 2014 15:24:16 +0000 In-Reply-To: <5481DAD4020000780004D3EA@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 Cc: Keir Fraser , Ian Jackson , Tim Deegan , Ian Campbell , xen-devel List-Id: xen-devel@lists.xenproject.org On 05/12/14 15:18, Jan Beulich wrote: >>>> On 05.12.14 at 16:01, wrote: >> On 05/12/14 14:47, Jan Beulich wrote: >>>>>> On 05.12.14 at 15:36, wrote: >>>> 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. >>> Not sure what you're asking for - why is removing the masking not >>> sufficient? >> There is no check to ensure that a non-preemptible arch_memoy_op is not >> called with a non-zero start_iter. >> >> This location needs something like >> >> if ( cmd & ~MEMOP_CMD_MASK ) >> return -ENOSYS; > I'm sorry - the default case of sub_arch_memory_op() will ensure > this. Ah - I see now. That is subtle. Better remember to double check the first patch which needs to add a preemptible subop. Reviewed-by: Andrew Cooper