From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v3 0/11] Rename/remove IS_PRIV Date: Mon, 22 Apr 2013 13:13:59 +0100 Message-ID: <51752987.1040507@eu.citrix.com> References: <1365800659-26040-1-git-send-email-dgdegra@tycho.nsa.gov> <5170299402000078000CEA6B@nat28.tlf.novell.com> <51701AF4.7010301@tycho.nsa.gov> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51701AF4.7010301@tycho.nsa.gov> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Daniel De Graaf Cc: Jan Beulich , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 18/04/13 17:10, Daniel De Graaf wrote: > On 04/18/2013 11:12 AM, Jan Beulich wrote: >>>>> On 12.04.13 at 23:04, Daniel De Graaf wrote: >>> Changes since v2: >>> - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately >>> - Use is_control_domain for CPUID >>> - Use is_{control,hardware}_domain for TSC >>> - MAINTAINERS patch included in series >>> >>> --- >>> >>> Following the conversion of most IS_PRIV hooks to XSM, the remaining >>> references to this function generally deal with direct hardware access >>> and not with the type of privilege checks that are best controlled by >>> XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain >>> and is used only when dealing with accesses that are both required by >>> dom0 and where it does not make sense to grant access to a domain other >>> than dom0. >>> >>> There are a number of existing places in the hypervisor that check >>> domain_id for equality to zero to make some distinction on dom0; this >>> series replaces these checks with is_hardware_domain to be consistent in >>> how the hypervisor checks a domain's access. >>> >>> Independent changes related to this series: >>> [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer >>> [PATCH 08/11] xen/cpupool: prevent a domain from moving itself >>> >>> Cleanup of IS_PRIV checks that should not be is_hardware_domain: >>> [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses >>> [PATCH 03/11] xen/xsm: add hooks for claim >>> [PATCH 04/11] hvm: convert access check for nested HVM to XSM >>> [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references >>> [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id >>> >>> Replace remaining calls to IS_PRIV: >>> [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain >>> >>> Use is_hardware_domain locations where (domid == 0) was used: >>> [PATCH 09/11] xen: use domid check in is_hardware_domain >>> [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks >>> [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0 >> While patch 1 went in a few days ago, patch 2 was held up just by >> XSA-46, which went public today. Consequently I also committed >> patch 2 a few minutes ago. > After looking at the XSA-46 patch, I think a the IS_PRIV removal can still > be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I'll > send the patch in a minute for comment. > >> For the rest of the series, however, I would want you two to work >> out the release related aspects, and I'd look into committing parts >> that I'm permitted to commit once I saw George's ack. > George: > > Here are my thoughts on the viability of these patches for a freeze > exception; I tried to address them in order from most to least important. > > Patch 8 fixes a bug with cpupools, although it is unlikely this bug will > be hit on a normal setup since it requires the cpupool operation to be > invoked from a domain other than dom0. It is also independent of the other > patches in this series, and I see no reason not to include it in 4.3. > > Patches 3-5 just change the behavior with XSM enabled, and makes these > access vectors more consistent with the rest of the hypervisor in the use > of XSM hooks when doing access checks related to domain permissions. I > think there is good reason to include these in 4.3, assuming there are no > technical objections. > > Patch 6 only changes ARM code, and should only change behavior with XSM > enabled. Since XSM on ARM is not really tested yet, this is less important > for 4.3 - although it allows removing some of the functions that were replaced > by XSM hooks, which may help avoid reintroducing users of these functions. > > Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0). > Since the hypervisor does not currently support setting is_privileged on > non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change. > The changes to .c files in patches 9-11 should produce identical code after > preprocessing and so have almost zero chance of introducing a bug. Remind me what the main benefit of this patch series is? Does this allow a greater degree of disaggregation? And, given that this is primarily about security, do you really want a brand new feature in users' hands with only 8 weeks of in-tree testing? -George