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 16:09:14 +0100 Message-ID: <5175529A.3090708@eu.citrix.com> References: <1365800659-26040-1-git-send-email-dgdegra@tycho.nsa.gov> <5170299402000078000CEA6B@nat28.tlf.novell.com> <51701AF4.7010301@tycho.nsa.gov> <51752987.1040507@eu.citrix.com> <517551A9.5050107@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: <517551A9.5050107@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 22/04/13 16:05, Daniel De Graaf wrote: > On 04/22/2013 08:13 AM, George Dunlap wrote: >> 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 > Patch 8 is a pure bugfix, not really security related except that it > fixes a deadlock from an interface usually restricted to dom0 only. > > The main benefit of the series is the increased granularity in control > for the additional access vectors, so that a disaggregated system can > limit each component to the minimal access it requires. Most of this > was already done in prior patches; the remainder here (patches 3-6) just > touches code that either wasn't in xen-unstable at the time I submitted > the original IS_PRIV->XSM conversion, or code that was left alone because > it was marked as a hack. > > Patches 7,9,10,11 are more function renames than code changes, and could > easily wait for 4.4 - the primary reason I posted them for 4.3 is to > avoid additional users of IS_PRIV being introduced during the freeze or > early in 4.4 (by IanC's suggestion), and because they are fairly trivial. > > Regarding only 8 weeks' testing: assuming the patch doesn't actually break > the logic it changes (I believe it doesn't, and it would be noticed rather > quickly if it did), the only change that it would require is adding the new > permissions to the XSM policy if they are used where they have not already > been added. This is something that users who modify the example policy > already need to do for other permissions as part of their changes. Thanks for the explanation. It sounds like these should mainly be low-risk changes, so re a release perspective: Acked-by: George Dunlap