From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH 11/18] xen: use XSM instead of IS_PRIV where duplicated Date: Mon, 06 Aug 2012 11:25:44 -0400 Message-ID: <501FE1F8.1040100@tycho.nsa.gov> References: <1344263550-3941-1-git-send-email-dgdegra@tycho.nsa.gov> <1344263550-3941-12-git-send-email-dgdegra@tycho.nsa.gov> <501FFC580200007800092FA4@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <501FFC580200007800092FA4@nat28.tlf.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: xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 08/06/2012 11:18 AM, Jan Beulich wrote: >>>> On 06.08.12 at 16:32, Daniel De Graaf wrote: >> --- a/xen/arch/x86/domctl.c >> +++ b/xen/arch/x86/domctl.c >> @@ -54,6 +54,26 @@ long arch_do_domctl( >> >> switch ( domctl->cmd ) >> { >> + /* TODO: the following do not have XSM hooks yet */ >> + case XEN_DOMCTL_set_cpuid: >> + case XEN_DOMCTL_suppress_spurious_page_faults: >> + case XEN_DOMCTL_debug_op: >> + case XEN_DOMCTL_gettscinfo: >> + case XEN_DOMCTL_settscinfo: >> + case XEN_DOMCTL_audit_p2m: >> + case XEN_DOMCTL_gdbsx_guestmemio: >> + case XEN_DOMCTL_gdbsx_pausevcpu: >> + case XEN_DOMCTL_gdbsx_unpausevcpu: >> + case XEN_DOMCTL_gdbsx_domstatus: >> + /* getpageframeinfo[23] will leak XEN_DOMCTL_PFINFO_XTAB on target GFNs */ > > Is that to state that the patch introduces a leak here? Or are you > trying to carefully tell us you spotted a problem in the existing > code? This is an information leak, not a memory leak. It's a (minor) problem with the placement of the existing XSM hooks allowing a domain to query information on remote domains. A later patch fixes this by adding an XSM hook for the entire query operation. >> + case XEN_DOMCTL_getpageframeinfo2: >> + case XEN_DOMCTL_getpageframeinfo3: >> + if ( !IS_PRIV(current->domain) ) >> + return -EPERM; >> + } >> + >> + switch ( domctl->cmd ) >> + { >> >> case XEN_DOMCTL_shadow_op: >> { >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -3366,12 +3366,12 @@ static int hvmop_set_pci_intx_level( >> if ( (op.domain > 0) || (op.bus > 0) || (op.device > 31) || (op.intx > 3) ) >> return -EINVAL; >> >> - rc = rcu_lock_remote_target_domain_by_id(op.domid, &d); >> - if ( rc != 0 ) >> - return rc; >> + d = rcu_lock_domain_by_id(op.domid); >> + if ( d == NULL ) >> + return -ESRCH; >> >> rc = -EINVAL; >> - if ( !is_hvm_domain(d) ) >> + if ( d == current->domain || !is_hvm_domain(d) ) > > What's wrong with rcu_lock_remote_target_domain_by_id() here > and in other places below? I think this minimally would deserve > a comment in the patch description, the more that this huge a > patch is already bad enough to look at. > > Jan > The main reason for this change is that rcu_lock_remote_target_domain_by_id calls IS_PRIV, and this patch is attempting to remove the duplicated calls. Would you prefer making another rcu_lock_* function that only checks against current->domain and doesn't include the IS_PRIV_FOR check? -- Daniel De Graaf National Security Agency