[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 0/11] Rename/remove IS_PRIV
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 <dgdegra@xxxxxxxxxxxxx> 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 <george.dunlap@xxxxxxxxxxxxx>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
