Add a new domctl sub-function to get data of a domain having changed
state (this is needed by Xenstore).
The returned state just contains the domid, the domain unique id,
and some flags (existing, shutdown, dying).
In order to enable Xenstore stubdom being built for multiple Xen
versions, make this domctl stable. For stable domctls the
interface_version is specific to the respective domctl op and it is an
in/out parameter: On input the caller is specifying the desired version
of the op, while on output the hypervisor will return the used version
(this will be at max the caller supplied version, but might be lower in
case the hypervisor doesn't support this version).
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V1:
- use a domctl subop for the new interface (Jan Beulich)
---
tools/flask/policy/modules/dom0.te | 2 +-
xen/common/domain.c | 51 +++++++++++++++++++++++++++++
xen/common/domctl.c | 19 ++++++++++-
xen/common/event_channel.c | 9 ++++-
xen/include/public/domctl.h | 33 +++++++++++++++++++
xen/include/xen/event.h | 6 ++++
xen/include/xen/sched.h | 2 ++
xen/include/xsm/dummy.h | 8 +++++
xen/include/xsm/xsm.h | 6 ++++
xen/xsm/dummy.c | 1 +
xen/xsm/flask/hooks.c | 7 ++++
xen/xsm/flask/policy/access_vectors | 2 ++
12 files changed, 143 insertions(+), 3 deletions(-)
diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index 16b8c9646d..6043c01b12 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
};
allow dom0_t dom0_t:domain2 {
set_cpu_policy gettsc settsc setscheduler set_vnumainfo
- get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
+ get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
};
allow dom0_t dom0_t:resource { add remove };
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 61b7899cb8..e55c6f6c5e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -154,6 +154,57 @@ void domain_reset_states(void)
rcu_read_unlock(&domlist_read_lock);
}
+static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
+ const struct domain *d)
+{
+ info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
+ if ( d->is_shut_down )
+ info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
+ if ( d->is_dying == DOMDYING_dead )
+ info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
+ info->unique_id = d->unique_id;
+}
+
+int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
+{
+ unsigned int dom;
+
+ memset(info, 0, sizeof(*info));
+
+ if ( d )
+ {
+ set_domain_state_info(info, d);
+
+ return 0;
+ }
+
+ while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
+ DOMID_FIRST_RESERVED )
+ {
+ d = rcu_lock_domain_by_id(dom);
+
+ if ( test_and_clear_bit(dom, dom_state_changed) )
+ {
+ info->domid = dom;
+ if ( d )
+ {
+ set_domain_state_info(info, d);
+
+ rcu_unlock_domain(d);
+ }
+
+ return 0;
+ }
+
+ if ( d )
+ {
+ rcu_unlock_domain(d);
+ }
+ }
+
+ return -ENOENT;
+}
+
static void __domain_finalise_shutdown(struct domain *d)
{
struct vcpu *v;
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ea16b75910..e030cce2ec 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -278,6 +278,11 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
return ERR_PTR(ret);
}
+static bool is_stable_domctl(uint32_t cmd)
+{
+ return cmd == XEN_DOMCTL_get_domain_state;
+}
+
long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
long ret = 0;
@@ -288,7 +293,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
if ( copy_from_guest(op, u_domctl, 1) )
return -EFAULT;
- if ( op->interface_version != XEN_DOMCTL_INTERFACE_VERSION )
+ if ( op->interface_version != XEN_DOMCTL_INTERFACE_VERSION &&
+ !is_stable_domctl(op->cmd) )
return -EACCES;
switch ( op->cmd )
@@ -309,6 +315,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
fallthrough;
case XEN_DOMCTL_test_assign_device:
case XEN_DOMCTL_vm_event_op:
+ case XEN_DOMCTL_get_domain_state:
if ( op->domain == DOMID_INVALID )
{
d = NULL;
@@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
__HYPERVISOR_domctl, "h", u_domctl);
break;
+ case XEN_DOMCTL_get_domain_state:
+ ret = xsm_get_domain_state(XSM_HOOK, d);
+ if ( ret )
+ break;
+
+ copyback = 1;
+ op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
+ ret = get_domain_state(&op->u.get_domain_state, d);
+ break;
+
default:
ret = arch_do_domctl(op, d, u_domctl);
break;
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 9b87d29968..13484b3e0d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
static DEFINE_SPINLOCK(global_virq_handlers_lock);
+struct domain *get_global_virq_handler(uint32_t virq)
+{
+ ASSERT(virq_is_global(virq));
+
+ return global_virq_handlers[virq] ?: hardware_domain;
+}
+
void send_global_virq(uint32_t virq)
{
ASSERT(virq_is_global(virq));
- send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq);
+ send_guest_global_virq(get_global_virq_handler(virq), virq);
}
int set_global_virq_handler(struct domain *d, uint32_t virq)
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 353f831e40..63607f7754 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -28,6 +28,7 @@
* Pure additions (e.g. new sub-commands) or compatible interface changes
* (e.g. adding semantics to 0-checked input fields or data to zeroed output
* fields) don't require a change of the version.
+ * Stable ops are NOT covered by XEN_DOMCTL_INTERFACE_VERSION!
*
* Last version bump: Xen 4.19
*/
@@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
};
#endif
+/*
+ * XEN_DOMCTL_get_domain_state (stable interface)
+ *
+ * Get state information of a domain.
+ *
+ * In case domain is DOMID_INVALID, return information about a domain having
+ * changed state and reset the state change indicator for that domain. This
+ * function is usable only by a domain having registered the VIRQ_DOM_EXC
+ * event (normally Xenstore).
+ *
+ * Supported interface versions: 0x00000000
+ */
+#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX 0
+struct xen_domctl_get_domain_state {
+ domid_t domid;
+ uint16_t state;
+#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST 0x0001 /* Domain is existing. */
+#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN 0x0002 /* Shutdown finished. */
+#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING 0x0004 /* Domain dying. */
+ uint32_t pad1; /* Returned as 0. */
+ uint64_t unique_id; /* Unique domain identifier. */
+ uint64_t pad2[6]; /* Returned as 0. */
+};
+
struct xen_domctl {
+/*
+ * Stable domctl ops:
+ * interface_version is per cmd, hypervisor can support multiple versions
+ * and will return used version (at max caller supplied value) in
+ * interface_version on return.
+ */
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
#define XEN_DOMCTL_destroydomain 2
@@ -1325,6 +1356,7 @@ struct xen_domctl {
#define XEN_DOMCTL_set_paging_mempool_size 86
#define XEN_DOMCTL_dt_overlay 87
#define XEN_DOMCTL_gsi_permission 88
+#define XEN_DOMCTL_get_domain_state 89 /* stable interface */
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1391,6 +1423,7 @@ struct xen_domctl {
#if defined(__arm__) || defined(__aarch64__)
struct xen_domctl_dt_overlay dt_overlay;
#endif
+ struct xen_domctl_get_domain_state get_domain_state;
uint8_t pad[128];
} u;
};
diff --git a/xen/include/xen/event.h b/xen/include/xen/event.h
index 48b79f3d62..f380668609 100644
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -43,6 +43,12 @@ void send_guest_global_virq(struct domain *d, uint32_t virq);
*/
int set_global_virq_handler(struct domain *d, uint32_t virq);
+/*
+ * get_global_virq_handler: Get domain handling a global virq.
+ * @virq: Virtual IRQ number (VIRQ_*), must be global
+ */
+struct domain *get_global_virq_handler(uint32_t virq);
+
/*
* send_guest_pirq:
* @d: Domain to which physical IRQ should be sent
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 667863263d..eb256de9d5 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -801,6 +801,8 @@ void domain_resume(struct domain *d);
int domain_soft_reset(struct domain *d, bool resuming);
void domain_reset_states(void);
+int get_domain_state(struct xen_domctl_get_domain_state *info,
+ struct domain *d);
int vcpu_start_shutdown_deferral(struct vcpu *v);
void vcpu_end_shutdown_deferral(struct vcpu *v);
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 7956f27a29..eaf5b05093 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -173,6 +173,7 @@ static XSM_INLINE int cf_check xsm_domctl(
case XEN_DOMCTL_unbind_pt_irq:
return xsm_default_action(XSM_DM_PRIV, current->domain, d);
case XEN_DOMCTL_getdomaininfo:
+ case XEN_DOMCTL_get_domain_state:
return xsm_default_action(XSM_XS_PRIV, current->domain, d);
default:
return xsm_default_action(XSM_PRIV, current->domain, d);
@@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
#endif /* CONFIG_ARGO */
+static XSM_INLINE int cf_check xsm_get_domain_state(
+ XSM_DEFAULT_ARG struct domain *d)
+{
+ XSM_ASSERT_ACTION(XSM_HOOK);
+ return xsm_default_action(action, current->domain, d);
+}
+
#include <public/version.h>
static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG uint32_t op)
{
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 627c0d2731..933b3889c9 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -201,6 +201,7 @@ struct xsm_ops {
int (*argo_register_any_source)(const struct domain *d);
int (*argo_send)(const struct domain *d, const struct domain *t);
#endif
+ int (*get_domain_state)(struct domain *d);
};
#ifdef CONFIG_XSM
@@ -775,6 +776,11 @@ static inline int xsm_argo_send(const struct domain *d, const struct domain *t)
#endif /* CONFIG_ARGO */
+static inline int xsm_get_domain_state(struct domain *d)
+{
+ return alternative_call(xsm_ops.get_domain_state, d);
+}
+
#endif /* XSM_NO_WRAPPERS */
#ifdef CONFIG_MULTIBOOT
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index e6ffa948f7..ce6fbdc6c5 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -148,6 +148,7 @@ static const struct xsm_ops __initconst_cf_clobber dummy_ops = {
.argo_register_any_source = xsm_argo_register_any_source,
.argo_send = xsm_argo_send,
#endif
+ .get_domain_state = xsm_get_domain_state,
};
void __init xsm_fixup_ops(struct xsm_ops *ops)
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index dfa23738cd..b1bad7e420 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -686,6 +686,7 @@ static int cf_check flask_domctl(struct domain *d, unsigned int cmd,
case XEN_DOMCTL_memory_mapping:
case XEN_DOMCTL_set_target:
case XEN_DOMCTL_vm_event_op:
+ case XEN_DOMCTL_get_domain_state:
/* These have individual XSM hooks (arch/../domctl.c) */
case XEN_DOMCTL_bind_pt_irq:
@@ -1853,6 +1854,11 @@ static int cf_check flask_argo_send(
#endif
+static int cf_check flask_get_domain_state(struct domain *d)
+{
+ return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GET_DOMAIN_STATE);
+}
+
static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.set_system_active = flask_set_system_active,
.security_domaininfo = flask_security_domaininfo,
@@ -1989,6 +1995,7 @@ static const struct xsm_ops __initconst_cf_clobber flask_ops = {
.argo_register_any_source = flask_argo_register_any_source,
.argo_send = flask_argo_send,
#endif
+ .get_domain_state = flask_get_domain_state,
};
const struct xsm_ops *__init flask_init(
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index a35e3d4c51..c9a8eeda4e 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -251,6 +251,8 @@ class domain2
resource_map
# XEN_DOMCTL_get_cpu_policy
get_cpu_policy
+# XEN_DOMCTL_get_domain_state
+ get_domain_state
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.43.0On 23.10.2024 15:10, Juergen Gross wrote:
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -154,6 +154,57 @@ void domain_reset_states(void)
> rcu_read_unlock(&domlist_read_lock);
> }
>
> +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
> + const struct domain *d)
> +{
> + info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
> + if ( d->is_shut_down )
> + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
> + if ( d->is_dying == DOMDYING_dead )
> + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
> + info->unique_id = d->unique_id;
> +}
> +
> +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
> +{
> + unsigned int dom;
> +
> + memset(info, 0, sizeof(*info));
Would this better go into set_domain_state_info()? Ah, no, you ...
> + if ( d )
> + {
> + set_domain_state_info(info, d);
> +
> + return 0;
> + }
> +
> + while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
> + DOMID_FIRST_RESERVED )
> + {
> + d = rcu_lock_domain_by_id(dom);
... acquiring the lock early and then ...
> + if ( test_and_clear_bit(dom, dom_state_changed) )
> + {
> + info->domid = dom;
> + if ( d )
> + {
> + set_domain_state_info(info, d);
... potentially bypassing the call (with just the domid set) requires it
that way.
As to the point in time when the lock is acquired: Why is that, seeing that
it complicates the unlocking a little, by requiring a 2nd unlock a few
lines down?
> + rcu_unlock_domain(d);
> + }
> +
> + return 0;
> + }
> +
> + if ( d )
> + {
> + rcu_unlock_domain(d);
> + }
Nit: No need for the braces.
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
>
> static DEFINE_SPINLOCK(global_virq_handlers_lock);
>
> +struct domain *get_global_virq_handler(uint32_t virq)
> +{
> + ASSERT(virq_is_global(virq));
> +
> + return global_virq_handlers[virq] ?: hardware_domain;
> +}
> +
> void send_global_virq(uint32_t virq)
> {
> ASSERT(virq_is_global(virq));
>
> - send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq);
> + send_guest_global_virq(get_global_virq_handler(virq), virq);
> }
Is this a stale leftover from an earlier version? There's no other caller of
get_global_virq_handler() here, hence the change looks unmotivated here.
> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
> };
> #endif
>
> +/*
> + * XEN_DOMCTL_get_domain_state (stable interface)
> + *
> + * Get state information of a domain.
> + *
> + * In case domain is DOMID_INVALID, return information about a domain having
> + * changed state and reset the state change indicator for that domain. This
> + * function is usable only by a domain having registered the VIRQ_DOM_EXC
> + * event (normally Xenstore).
> + *
> + * Supported interface versions: 0x00000000
> + */
> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX 0
> +struct xen_domctl_get_domain_state {
> + domid_t domid;
Despite the DOMID_INVALID special case the redundant domid here is odd.
You actually add the new sub-op to the special casing of op->domain at the
top of do_domctl(), so the sole difference to most other sub-ops would be
that this then is an IN/OUT (rather than the field here being an output
only when DOMID_INVALID was passed in via the common domid field).
> + uint16_t state;
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST 0x0001 /* Domain is existing. */
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN 0x0002 /* Shutdown finished. */
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING 0x0004 /* Domain dying. */
> + uint32_t pad1; /* Returned as 0. */
> + uint64_t unique_id; /* Unique domain identifier. */
> + uint64_t pad2[6]; /* Returned as 0. */
> +};
What are the intentions with this padding array?
Jan
On 31.10.24 12:16, Jan Beulich wrote:
> On 23.10.2024 15:10, Juergen Gross wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -154,6 +154,57 @@ void domain_reset_states(void)
>> rcu_read_unlock(&domlist_read_lock);
>> }
>>
>> +static void set_domain_state_info(struct xen_domctl_get_domain_state *info,
>> + const struct domain *d)
>> +{
>> + info->state = XEN_DOMCTL_GETDOMSTATE_STATE_EXIST;
>> + if ( d->is_shut_down )
>> + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN;
>> + if ( d->is_dying == DOMDYING_dead )
>> + info->state |= XEN_DOMCTL_GETDOMSTATE_STATE_DYING;
>> + info->unique_id = d->unique_id;
>> +}
>> +
>> +int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d)
>> +{
>> + unsigned int dom;
>> +
>> + memset(info, 0, sizeof(*info));
>
> Would this better go into set_domain_state_info()? Ah, no, you ...
>
>> + if ( d )
>> + {
>> + set_domain_state_info(info, d);
>> +
>> + return 0;
>> + }
>> +
>> + while ( (dom = find_first_bit(dom_state_changed, DOMID_MASK + 1)) <
>> + DOMID_FIRST_RESERVED )
>> + {
>> + d = rcu_lock_domain_by_id(dom);
>
> ... acquiring the lock early and then ...
>
>> + if ( test_and_clear_bit(dom, dom_state_changed) )
>> + {
>> + info->domid = dom;
>> + if ( d )
>> + {
>> + set_domain_state_info(info, d);
>
> ... potentially bypassing the call (with just the domid set) requires it
> that way.
>
> As to the point in time when the lock is acquired: Why is that, seeing that
> it complicates the unlocking a little, by requiring a 2nd unlock a few
> lines down?
I agree this can be simplified.
>
>> + rcu_unlock_domain(d);
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + if ( d )
>> + {
>> + rcu_unlock_domain(d);
>> + }
>
> Nit: No need for the braces.
>
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -969,11 +969,18 @@ static struct domain *global_virq_handlers[NR_VIRQS] __read_mostly;
>>
>> static DEFINE_SPINLOCK(global_virq_handlers_lock);
>>
>> +struct domain *get_global_virq_handler(uint32_t virq)
>> +{
>> + ASSERT(virq_is_global(virq));
>> +
>> + return global_virq_handlers[virq] ?: hardware_domain;
>> +}
>> +
>> void send_global_virq(uint32_t virq)
>> {
>> ASSERT(virq_is_global(virq));
>>
>> - send_guest_global_virq(global_virq_handlers[virq] ?: hardware_domain, virq);
>> + send_guest_global_virq(get_global_virq_handler(virq), virq);
>> }
>
> Is this a stale leftover from an earlier version? There's no other caller of
> get_global_virq_handler() here, hence the change looks unmotivated here.
I think it is indeed stale now.
>
>> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>> };
>> #endif
>>
>> +/*
>> + * XEN_DOMCTL_get_domain_state (stable interface)
>> + *
>> + * Get state information of a domain.
>> + *
>> + * In case domain is DOMID_INVALID, return information about a domain having
>> + * changed state and reset the state change indicator for that domain. This
>> + * function is usable only by a domain having registered the VIRQ_DOM_EXC
>> + * event (normally Xenstore).
>> + *
>> + * Supported interface versions: 0x00000000
>> + */
>> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX 0
>> +struct xen_domctl_get_domain_state {
>> + domid_t domid;
>
> Despite the DOMID_INVALID special case the redundant domid here is odd.
> You actually add the new sub-op to the special casing of op->domain at the
> top of do_domctl(), so the sole difference to most other sub-ops would be
> that this then is an IN/OUT (rather than the field here being an output
> only when DOMID_INVALID was passed in via the common domid field).
The main idea was to have all data returned by get_domain_state() in struct
xen_domctl_get_domain_state. I'm fine either way.
But I think this discussion is moot now, as it seems we are switching to a
new hypercall anyway, where we probably will have all data in per sub-op
structs.
>
>> + uint16_t state;
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST 0x0001 /* Domain is existing. */
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN 0x0002 /* Shutdown finished. */
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING 0x0004 /* Domain dying. */
>> + uint32_t pad1; /* Returned as 0. */
>> + uint64_t unique_id; /* Unique domain identifier. */
>> + uint64_t pad2[6]; /* Returned as 0. */
>> +};
>
> What are the intentions with this padding array?
The idea was to allow to return additional domain data in future without having
to extend the struct.
Juergen
On 23.10.2024 15:10, Juergen Gross wrote:
> Add a new domctl sub-function to get data of a domain having changed
> state (this is needed by Xenstore).
>
> The returned state just contains the domid, the domain unique id,
> and some flags (existing, shutdown, dying).
>
> In order to enable Xenstore stubdom being built for multiple Xen
> versions, make this domctl stable. For stable domctls the
> interface_version is specific to the respective domctl op and it is an
> in/out parameter: On input the caller is specifying the desired version
> of the op, while on output the hypervisor will return the used version
> (this will be at max the caller supplied version, but might be lower in
> case the hypervisor doesn't support this version).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V1:
> - use a domctl subop for the new interface (Jan Beulich)
Just as a preliminary reply: With Andrew's stabilization plans I'm not
sure this, in particular including ...
> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
> };
> #endif
>
> +/*
> + * XEN_DOMCTL_get_domain_state (stable interface)
> + *
> + * Get state information of a domain.
> + *
> + * In case domain is DOMID_INVALID, return information about a domain having
> + * changed state and reset the state change indicator for that domain. This
> + * function is usable only by a domain having registered the VIRQ_DOM_EXC
> + * event (normally Xenstore).
> + *
> + * Supported interface versions: 0x00000000
> + */
> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX 0
> +struct xen_domctl_get_domain_state {
> + domid_t domid;
> + uint16_t state;
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST 0x0001 /* Domain is existing. */
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN 0x0002 /* Shutdown finished. */
> +#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING 0x0004 /* Domain dying. */
> + uint32_t pad1; /* Returned as 0. */
> + uint64_t unique_id; /* Unique domain identifier. */
> + uint64_t pad2[6]; /* Returned as 0. */
> +};
> +
> struct xen_domctl {
> +/*
> + * Stable domctl ops:
> + * interface_version is per cmd, hypervisor can support multiple versions
> + * and will return used version (at max caller supplied value) in
> + * interface_version on return.
> + */
... this (ab)use of the interface version, is the way to go. I think
we want a brand new hypercall, with stability just like most other
hypercalls have it. Backwards-incompatible interface changes then
simply aren't allowed (as in: require a new sub-op instead).
Jan
On 28.10.24 11:50, Jan Beulich wrote:
> On 23.10.2024 15:10, Juergen Gross wrote:
>> Add a new domctl sub-function to get data of a domain having changed
>> state (this is needed by Xenstore).
>>
>> The returned state just contains the domid, the domain unique id,
>> and some flags (existing, shutdown, dying).
>>
>> In order to enable Xenstore stubdom being built for multiple Xen
>> versions, make this domctl stable. For stable domctls the
>> interface_version is specific to the respective domctl op and it is an
>> in/out parameter: On input the caller is specifying the desired version
>> of the op, while on output the hypervisor will return the used version
>> (this will be at max the caller supplied version, but might be lower in
>> case the hypervisor doesn't support this version).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V1:
>> - use a domctl subop for the new interface (Jan Beulich)
>
> Just as a preliminary reply: With Andrew's stabilization plans I'm not
> sure this, in particular including ...
>
>> @@ -1236,7 +1237,37 @@ struct xen_domctl_dt_overlay {
>> };
>> #endif
>>
>> +/*
>> + * XEN_DOMCTL_get_domain_state (stable interface)
>> + *
>> + * Get state information of a domain.
>> + *
>> + * In case domain is DOMID_INVALID, return information about a domain having
>> + * changed state and reset the state change indicator for that domain. This
>> + * function is usable only by a domain having registered the VIRQ_DOM_EXC
>> + * event (normally Xenstore).
>> + *
>> + * Supported interface versions: 0x00000000
>> + */
>> +#define XEN_DOMCTL_GETDOMSTATE_VERS_MAX 0
>> +struct xen_domctl_get_domain_state {
>> + domid_t domid;
>> + uint16_t state;
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_EXIST 0x0001 /* Domain is existing. */
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_SHUTDOWN 0x0002 /* Shutdown finished. */
>> +#define XEN_DOMCTL_GETDOMSTATE_STATE_DYING 0x0004 /* Domain dying. */
>> + uint32_t pad1; /* Returned as 0. */
>> + uint64_t unique_id; /* Unique domain identifier. */
>> + uint64_t pad2[6]; /* Returned as 0. */
>> +};
>> +
>> struct xen_domctl {
>> +/*
>> + * Stable domctl ops:
>> + * interface_version is per cmd, hypervisor can support multiple versions
>> + * and will return used version (at max caller supplied value) in
>> + * interface_version on return.
>> + */
>
> ... this (ab)use of the interface version, is the way to go. I think
> we want a brand new hypercall, with stability just like most other
> hypercalls have it. Backwards-incompatible interface changes then
> simply aren't allowed (as in: require a new sub-op instead).
Okay, so back to my initial RFC variant then. :-)
Andrew, can you confirm you'd prefer that route?
Juergen
On 10/23/24 09:10, Juergen Gross wrote:
> Add a new domctl sub-function to get data of a domain having changed
> state (this is needed by Xenstore).
>
> The returned state just contains the domid, the domain unique id,
> and some flags (existing, shutdown, dying).
>
> In order to enable Xenstore stubdom being built for multiple Xen
> versions, make this domctl stable. For stable domctls the
> interface_version is specific to the respective domctl op and it is an
> in/out parameter: On input the caller is specifying the desired version
> of the op, while on output the hypervisor will return the used version
> (this will be at max the caller supplied version, but might be lower in
> case the hypervisor doesn't support this version).
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V1:
> - use a domctl subop for the new interface (Jan Beulich)
> ---
> tools/flask/policy/modules/dom0.te | 2 +-
> xen/common/domain.c | 51 +++++++++++++++++++++++++++++
> xen/common/domctl.c | 19 ++++++++++-
> xen/common/event_channel.c | 9 ++++-
> xen/include/public/domctl.h | 33 +++++++++++++++++++
> xen/include/xen/event.h | 6 ++++
> xen/include/xen/sched.h | 2 ++
> xen/include/xsm/dummy.h | 8 +++++
> xen/include/xsm/xsm.h | 6 ++++
> xen/xsm/dummy.c | 1 +
> xen/xsm/flask/hooks.c | 7 ++++
> xen/xsm/flask/policy/access_vectors | 2 ++
> 12 files changed, 143 insertions(+), 3 deletions(-)
>
> diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
> index 16b8c9646d..6043c01b12 100644
> --- a/tools/flask/policy/modules/dom0.te
> +++ b/tools/flask/policy/modules/dom0.te
> @@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
> };
> allow dom0_t dom0_t:domain2 {
> set_cpu_policy gettsc settsc setscheduler set_vnumainfo
> - get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
> + get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
I don't think that is where you want it, as that restricts dom0 to only
being able to make that call against dom0. The question I have is, are
you looking for this permission to be explicitly assigned to dom0 or to
the domain type that was allowed to create the domain. IMHO, I think you
would want the latter, so not only should the permission go here, but
also added to xen.if:create_domain_common.
Additionally, I would also recommend adding the following to xenstore.te:
allow xenstore_t domain_type:domain get_domain_state
> allow dom0_t dom0_t:resource { add remove };
>
...
> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
> __HYPERVISOR_domctl, "h", u_domctl);
> break;
>
> + case XEN_DOMCTL_get_domain_state:
> + ret = xsm_get_domain_state(XSM_HOOK, d);
XSM_HOOK will allow any domain to make this call on any domain. What I
think you want here is XSM_XS_PRIV. That will allow either a domain
flagged as the xenstore domain or dom0 to make the call.
> + if ( ret )
> + break;
> +
> + copyback = 1;
> + op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
> + ret = get_domain_state(&op->u.get_domain_state, d);
> + break;
> +
> default:
> ret = arch_do_domctl(op, d, u_domctl);
> break;
...
> @@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
>
> #endif /* CONFIG_ARGO */
>
> +static XSM_INLINE int cf_check xsm_get_domain_state(
> + XSM_DEFAULT_ARG struct domain *d)
> +{
> + XSM_ASSERT_ACTION(XSM_HOOK);
Per the above, this would need changed to XSM_XS_PRIV.
> + return xsm_default_action(action, current->domain, d);
> +}
> +
> #include <public/version.h>
> static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG uint32_t op)
> {
...
> @@ -1853,6 +1854,11 @@ static int cf_check flask_argo_send(
>
> #endif
>
> +static int cf_check flask_get_domain_state(struct domain *d)
> +{
> + return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GET_DOMAIN_STATE);
I believe you want SECCLASS_DOMAIN2 here.
> +}
> +
> static const struct xsm_ops __initconst_cf_clobber flask_ops = {
> .set_system_active = flask_set_system_active,
> .security_domaininfo = flask_security_domaininfo,
v/r,
dps
On 23.10.24 17:55, Daniel P. Smith wrote:
> On 10/23/24 09:10, Juergen Gross wrote:
>> Add a new domctl sub-function to get data of a domain having changed
>> state (this is needed by Xenstore).
>>
>> The returned state just contains the domid, the domain unique id,
>> and some flags (existing, shutdown, dying).
>>
>> In order to enable Xenstore stubdom being built for multiple Xen
>> versions, make this domctl stable. For stable domctls the
>> interface_version is specific to the respective domctl op and it is an
>> in/out parameter: On input the caller is specifying the desired version
>> of the op, while on output the hypervisor will return the used version
>> (this will be at max the caller supplied version, but might be lower in
>> case the hypervisor doesn't support this version).
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V1:
>> - use a domctl subop for the new interface (Jan Beulich)
>> ---
>> tools/flask/policy/modules/dom0.te | 2 +-
>> xen/common/domain.c | 51 +++++++++++++++++++++++++++++
>> xen/common/domctl.c | 19 ++++++++++-
>> xen/common/event_channel.c | 9 ++++-
>> xen/include/public/domctl.h | 33 +++++++++++++++++++
>> xen/include/xen/event.h | 6 ++++
>> xen/include/xen/sched.h | 2 ++
>> xen/include/xsm/dummy.h | 8 +++++
>> xen/include/xsm/xsm.h | 6 ++++
>> xen/xsm/dummy.c | 1 +
>> xen/xsm/flask/hooks.c | 7 ++++
>> xen/xsm/flask/policy/access_vectors | 2 ++
>> 12 files changed, 143 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/flask/policy/modules/dom0.te
>> b/tools/flask/policy/modules/dom0.te
>> index 16b8c9646d..6043c01b12 100644
>> --- a/tools/flask/policy/modules/dom0.te
>> +++ b/tools/flask/policy/modules/dom0.te
>> @@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
>> };
>> allow dom0_t dom0_t:domain2 {
>> set_cpu_policy gettsc settsc setscheduler set_vnumainfo
>> - get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
>> + get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
>
> I don't think that is where you want it, as that restricts dom0 to only being
> able to make that call against dom0. The question I have is, are you looking for
> this permission to be explicitly assigned to dom0 or to the domain type that was
> allowed to create the domain. IMHO, I think you would want the latter, so not
> only should the permission go here, but also added to xen.if:create_domain_common.
>
> Additionally, I would also recommend adding the following to xenstore.te:
>
> allow xenstore_t domain_type:domain get_domain_state
Okay, but shouldn't this be:
allow xenstore_t domain_type:domain2 get_domain_state;
>
>> allow dom0_t dom0_t:resource { add remove };
>
> ...
>
>> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>> u_domctl)
>> __HYPERVISOR_domctl, "h", u_domctl);
>> break;
>> + case XEN_DOMCTL_get_domain_state:
>> + ret = xsm_get_domain_state(XSM_HOOK, d);
>
> XSM_HOOK will allow any domain to make this call on any domain. What I think you
> want here is XSM_XS_PRIV. That will allow either a domain flagged as the
> xenstore domain or dom0 to make the call.
I thought so, too, but looking at the "getdomaininfo" example it seems
to be okay this way, too. Especially with the addition to xsm_domctl()
checking for XSM_XS_PRIV.
>
>> + if ( ret )
>> + break;
>> +
>> + copyback = 1;
>> + op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
>> + ret = get_domain_state(&op->u.get_domain_state, d);
>> + break;
>> +
>> default:
>> ret = arch_do_domctl(op, d, u_domctl);
>> break;
>
> ...
>
>> @@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
>> #endif /* CONFIG_ARGO */
>> +static XSM_INLINE int cf_check xsm_get_domain_state(
>> + XSM_DEFAULT_ARG struct domain *d)
>> +{
>> + XSM_ASSERT_ACTION(XSM_HOOK);
>
> Per the above, this would need changed to XSM_XS_PRIV.
>
>> + return xsm_default_action(action, current->domain, d);
>> +}
>> +
>> #include <public/version.h>
>> static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG uint32_t op)
>> {
>
> ...
>
>> @@ -1853,6 +1854,11 @@ static int cf_check flask_argo_send(
>> #endif
>> +static int cf_check flask_get_domain_state(struct domain *d)
>> +{
>> + return current_has_perm(d, SECCLASS_DOMAIN, DOMAIN__GET_DOMAIN_STATE);
>
> I believe you want SECCLASS_DOMAIN2 here.
Oh, indeed. And probably DOMAIN2__GET_DOMAIN_STATE
Thanks,
Juergen
On 10/24/24 05:13, Jürgen Groß wrote:
> On 23.10.24 17:55, Daniel P. Smith wrote:
>> On 10/23/24 09:10, Juergen Gross wrote:
>>> Add a new domctl sub-function to get data of a domain having changed
>>> state (this is needed by Xenstore).
>>>
>>> The returned state just contains the domid, the domain unique id,
>>> and some flags (existing, shutdown, dying).
>>>
>>> In order to enable Xenstore stubdom being built for multiple Xen
>>> versions, make this domctl stable. For stable domctls the
>>> interface_version is specific to the respective domctl op and it is an
>>> in/out parameter: On input the caller is specifying the desired version
>>> of the op, while on output the hypervisor will return the used version
>>> (this will be at max the caller supplied version, but might be lower in
>>> case the hypervisor doesn't support this version).
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> V1:
>>> - use a domctl subop for the new interface (Jan Beulich)
>>> ---
>>> tools/flask/policy/modules/dom0.te | 2 +-
>>> xen/common/domain.c | 51 +++++++++++++++++++++++++++++
>>> xen/common/domctl.c | 19 ++++++++++-
>>> xen/common/event_channel.c | 9 ++++-
>>> xen/include/public/domctl.h | 33 +++++++++++++++++++
>>> xen/include/xen/event.h | 6 ++++
>>> xen/include/xen/sched.h | 2 ++
>>> xen/include/xsm/dummy.h | 8 +++++
>>> xen/include/xsm/xsm.h | 6 ++++
>>> xen/xsm/dummy.c | 1 +
>>> xen/xsm/flask/hooks.c | 7 ++++
>>> xen/xsm/flask/policy/access_vectors | 2 ++
>>> 12 files changed, 143 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tools/flask/policy/modules/dom0.te
>>> b/tools/flask/policy/modules/dom0.te
>>> index 16b8c9646d..6043c01b12 100644
>>> --- a/tools/flask/policy/modules/dom0.te
>>> +++ b/tools/flask/policy/modules/dom0.te
>>> @@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
>>> };
>>> allow dom0_t dom0_t:domain2 {
>>> set_cpu_policy gettsc settsc setscheduler set_vnumainfo
>>> - get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
>>> + get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
>>
>> I don't think that is where you want it, as that restricts dom0 to
>> only being able to make that call against dom0. The question I have
>> is, are you looking for this permission to be explicitly assigned to
>> dom0 or to the domain type that was allowed to create the domain.
>> IMHO, I think you would want the latter, so not only should the
>> permission go here, but also added to xen.if:create_domain_common.
>>
>> Additionally, I would also recommend adding the following to xenstore.te:
>>
>> allow xenstore_t domain_type:domain get_domain_state
>
> Okay, but shouldn't this be:
>
> allow xenstore_t domain_type:domain2 get_domain_state;
Apologies, yes that was a typo on my part.
>>
>>> allow dom0_t dom0_t:resource { add remove };
>>
>> ...
>>
>>> @@ -866,6 +873,16 @@ long
>>> do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>> __HYPERVISOR_domctl, "h", u_domctl);
>>> break;
>>> + case XEN_DOMCTL_get_domain_state:
>>> + ret = xsm_get_domain_state(XSM_HOOK, d);
>>
>> XSM_HOOK will allow any domain to make this call on any domain. What I
>> think you want here is XSM_XS_PRIV. That will allow either a domain
>> flagged as the xenstore domain or dom0 to make the call.
>
> I thought so, too, but looking at the "getdomaininfo" example it seems
> to be okay this way, too. Especially with the addition to xsm_domctl()
> checking for XSM_XS_PRIV.
I know this has been done in the past, but imho it is not a very good
practice. Access checks really should always restrict equal or down.
There should be a strong reason, which probably would deserves a code
comment, to allow a check to relax up. Restricting equal should not
reduce access, and if it does, then it means there is an unintended
access path which is now exposed.
>>
>>> + if ( ret )
>>> + break;
>>> +
>>> + copyback = 1;
>>> + op->interface_version = XEN_DOMCTL_GETDOMSTATE_VERS_MAX;
>>> + ret = get_domain_state(&op->u.get_domain_state, d);
>>> + break;
>>> +
>>> default:
>>> ret = arch_do_domctl(op, d, u_domctl);
>>> break;
>>
>> ...
>>
>>> @@ -815,6 +816,13 @@ static XSM_INLINE int cf_check xsm_argo_send(
>>> #endif /* CONFIG_ARGO */
>>> +static XSM_INLINE int cf_check xsm_get_domain_state(
>>> + XSM_DEFAULT_ARG struct domain *d)
>>> +{
>>> + XSM_ASSERT_ACTION(XSM_HOOK);
>>
>> Per the above, this would need changed to XSM_XS_PRIV.
>>
>>> + return xsm_default_action(action, current->domain, d);
>>> +}
>>> +
>>> #include <public/version.h>
>>> static XSM_INLINE int cf_check xsm_xen_version(XSM_DEFAULT_ARG
>>> uint32_t op)
>>> {
>>
>> ...
>>
>>> @@ -1853,6 +1854,11 @@ static int cf_check flask_argo_send(
>>> #endif
>>> +static int cf_check flask_get_domain_state(struct domain *d)
>>> +{
>>> + return current_has_perm(d, SECCLASS_DOMAIN,
>>> DOMAIN__GET_DOMAIN_STATE);
>>
>> I believe you want SECCLASS_DOMAIN2 here.
>
> Oh, indeed. And probably DOMAIN2__GET_DOMAIN_STATE
>
>
> Thanks,
No problem.
v/r,
dps
On 24.10.24 15:35, Daniel P. Smith wrote:
> On 10/24/24 05:13, Jürgen Groß wrote:
>> On 23.10.24 17:55, Daniel P. Smith wrote:
>>> On 10/23/24 09:10, Juergen Gross wrote:
>>>> Add a new domctl sub-function to get data of a domain having changed
>>>> state (this is needed by Xenstore).
>>>>
>>>> The returned state just contains the domid, the domain unique id,
>>>> and some flags (existing, shutdown, dying).
>>>>
>>>> In order to enable Xenstore stubdom being built for multiple Xen
>>>> versions, make this domctl stable. For stable domctls the
>>>> interface_version is specific to the respective domctl op and it is an
>>>> in/out parameter: On input the caller is specifying the desired version
>>>> of the op, while on output the hypervisor will return the used version
>>>> (this will be at max the caller supplied version, but might be lower in
>>>> case the hypervisor doesn't support this version).
>>>>
>>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>>> ---
>>>> V1:
>>>> - use a domctl subop for the new interface (Jan Beulich)
>>>> ---
>>>> tools/flask/policy/modules/dom0.te | 2 +-
>>>> xen/common/domain.c | 51 +++++++++++++++++++++++++++++
>>>> xen/common/domctl.c | 19 ++++++++++-
>>>> xen/common/event_channel.c | 9 ++++-
>>>> xen/include/public/domctl.h | 33 +++++++++++++++++++
>>>> xen/include/xen/event.h | 6 ++++
>>>> xen/include/xen/sched.h | 2 ++
>>>> xen/include/xsm/dummy.h | 8 +++++
>>>> xen/include/xsm/xsm.h | 6 ++++
>>>> xen/xsm/dummy.c | 1 +
>>>> xen/xsm/flask/hooks.c | 7 ++++
>>>> xen/xsm/flask/policy/access_vectors | 2 ++
>>>> 12 files changed, 143 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/tools/flask/policy/modules/dom0.te
>>>> b/tools/flask/policy/modules/dom0.te
>>>> index 16b8c9646d..6043c01b12 100644
>>>> --- a/tools/flask/policy/modules/dom0.te
>>>> +++ b/tools/flask/policy/modules/dom0.te
>>>> @@ -40,7 +40,7 @@ allow dom0_t dom0_t:domain {
>>>> };
>>>> allow dom0_t dom0_t:domain2 {
>>>> set_cpu_policy gettsc settsc setscheduler set_vnumainfo
>>>> - get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy
>>>> + get_vnumainfo psr_cmt_op psr_alloc get_cpu_policy get_domain_state
>>>
>>> I don't think that is where you want it, as that restricts dom0 to only being
>>> able to make that call against dom0. The question I have is, are you looking
>>> for this permission to be explicitly assigned to dom0 or to the domain type
>>> that was allowed to create the domain. IMHO, I think you would want the
>>> latter, so not only should the permission go here, but also added to
>>> xen.if:create_domain_common.
>>>
>>> Additionally, I would also recommend adding the following to xenstore.te:
>>>
>>> allow xenstore_t domain_type:domain get_domain_state
>>
>> Okay, but shouldn't this be:
>>
>> allow xenstore_t domain_type:domain2 get_domain_state;
>
> Apologies, yes that was a typo on my part.
>
>>>
>>>> allow dom0_t dom0_t:resource { add remove };
>>>
>>> ...
>>>
>>>> @@ -866,6 +873,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t)
>>>> u_domctl)
>>>> __HYPERVISOR_domctl, "h", u_domctl);
>>>> break;
>>>> + case XEN_DOMCTL_get_domain_state:
>>>> + ret = xsm_get_domain_state(XSM_HOOK, d);
>>>
>>> XSM_HOOK will allow any domain to make this call on any domain. What I think
>>> you want here is XSM_XS_PRIV. That will allow either a domain flagged as the
>>> xenstore domain or dom0 to make the call.
>>
>> I thought so, too, but looking at the "getdomaininfo" example it seems
>> to be okay this way, too. Especially with the addition to xsm_domctl()
>> checking for XSM_XS_PRIV.
>
> I know this has been done in the past, but imho it is not a very good practice.
> Access checks really should always restrict equal or down. There should be a
> strong reason, which probably would deserves a code comment, to allow a check to
> relax up. Restricting equal should not reduce access, and if it does, then it
> means there is an unintended access path which is now exposed.
Sounds logical.
I'll check if it works the way you are suggesting. If so, I'll send a patch
fixing the getdomaininfo case.
Juergen
© 2016 - 2024 Red Hat, Inc.