Re: [PATCH v3 0/11] Rename/remove IS_PRIV

[George Dunlap <george.dunlap@eu.citrix.com>]

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 <dgdegra@tycho.nsa.gov> 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