When using XSM Flask, passing DOMID_INVALID will result in a NULL pointer reference from the passing of NULL as the target domain to xsm_get_domain_state(). Simply not invoking xsm_get_domain_state() when the target domain is NULL opens the opportunity to circumvent the XSM get_domain_state access check. This is due to the fact that the call to xsm_domctl() for get_domain_state op is a no-op check, deferring to xsm_get_domain_state(). Modify the helper get_domain_state() to ensure the requesting domain has get_domain_state access for the target domain, whether the target domain is explicitly set or implicitly determined with a domain state search. In the case of access not being allowed for a domain found during an implicit search, the search will continue to the next domain whose state has changed. Fixes: 3ad3df1bd0aa ("xen: add new domctl get_domain_state") Reported-by: Chris Rogers <rogersc@ainfosec.com> Reported-by: Dmytro Firsov <dmytro_firsov@epam.com> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- Changes in v2: - fix commit message - init dom as -1 - rework loop logic to use test_and_clear_bit() --- xen/common/domain.c | 27 +++++++++++++++++++++------ xen/common/domctl.c | 7 ++----- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/xen/common/domain.c b/xen/common/domain.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -XXX,XX +XXX,XX @@ static void set_domain_state_info(struct xen_domctl_get_domain_state *info, int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, domid_t *domid) { - unsigned int dom; + unsigned int dom = -1; int rc = -ENOENT; struct domain *hdl; @@ -XXX,XX +XXX,XX @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, if ( d ) { + rc = xsm_get_domain_state(XSM_XS_PRIV, d); + if ( rc ) + return rc; + set_domain_state_info(info, d); return 0; @@ -XXX,XX +XXX,XX @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, while ( dom_state_changed ) { - dom = find_first_bit(dom_state_changed, DOMID_MASK + 1); + dom = find_next_bit(dom_state_changed, DOMID_MASK + 1, dom + 1); if ( dom >= DOMID_FIRST_RESERVED ) break; + + d = rcu_lock_domain_by_id(dom); + if ( d && xsm_get_domain_state(XSM_XS_PRIV, d) ) + { + rcu_unlock_domain(d); + d = NULL; + continue; + } + if ( test_and_clear_bit(dom, dom_state_changed) ) { *domid = dom; - d = rcu_lock_domain_by_id(dom); - if ( d ) { set_domain_state_info(info, d); - rcu_unlock_domain(d); } else memset(info, 0, sizeof(*info)); rc = 0; - break; } + + if ( d ) + { + rcu_unlock_domain(d); + d = NULL; + } } out: diff --git a/xen/common/domctl.c b/xen/common/domctl.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -XXX,XX +XXX,XX @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_get_domain_state: - ret = xsm_get_domain_state(XSM_XS_PRIV, d); - if ( ret ) - break; - - copyback = 1; ret = get_domain_state(&op->u.get_domain_state, d, &op->domain); + if ( !ret ) + copyback = 1; break; default: -- 2.39.5 When using XSM Flask, passing DOMID_INVALID will result in a NULL pointer reference from the passing of NULL as the target domain to xsm_get_domain_state(). Simply not invoking xsm_get_domain_state() when the target domain is NULL opens the opportunity to circumvent the XSM get_domain_state access check. This is due to the fact that the call to xsm_domctl() for get_domain_state op is a no-op check, deferring to xsm_get_domain_state(). Modify the helper get_domain_state() to ensure the requesting domain has get_domain_state access for the target domain, whether the target domain is explicitly set or implicitly determined with a domain state search. In the case of access not being allowed for a domain found during an implicit search, the search will continue to the next domain whose state has changed. Fixes: 3ad3df1bd0aa ("xen: add new domctl get_domain_state") Reported-by: Chris Rogers <rogersc@ainfosec.com> Reported-by: Dmytro Firsov <dmytro_firsov@epam.com> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- Changes in v3: - collapse if statements - dropped unnecessary NULL - use true for copyback Changes in v2: - fix commit message - init dom as -1 - rework loop logic to use test_and_clear_bit() --- xen/common/domain.c | 37 ++++++++++++++++++++++--------------- xen/common/domctl.c | 7 ++----- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/xen/common/domain.c b/xen/common/domain.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -XXX,XX +XXX,XX @@ static void set_domain_state_info(struct xen_domctl_get_domain_state *info, int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, domid_t *domid) { - unsigned int dom; + unsigned int dom = -1; int rc = -ENOENT; struct domain *hdl; @@ -XXX,XX +XXX,XX @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, if ( d ) { + rc = xsm_get_domain_state(XSM_XS_PRIV, d); + if ( rc ) + return rc; + set_domain_state_info(info, d); return 0; @@ -XXX,XX +XXX,XX @@ int get_domain_state(struct xen_domctl_get_domain_state *info, struct domain *d, while ( dom_state_changed ) { - dom = find_first_bit(dom_state_changed, DOMID_MASK + 1); + dom = find_next_bit(dom_state_changed, DOMID_MASK + 1, dom + 1); if ( dom >= DOMID_FIRST_RESERVED ) break; - if ( test_and_clear_bit(dom, dom_state_changed) ) - { - *domid = dom; - - d = rcu_lock_domain_by_id(dom); + d = rcu_lock_domain_by_id(dom); + if ( (d && xsm_get_domain_state(XSM_XS_PRIV, d)) || + !test_and_clear_bit(dom, dom_state_changed) ) + { if ( d ) - { - set_domain_state_info(info, d); - rcu_unlock_domain(d); - } - else - memset(info, 0, sizeof(*info)); + continue; + } - rc = 0; + *domid = dom; - break; + if ( d ) + { + set_domain_state_info(info, d); + rcu_unlock_domain(d); } + else + memset(info, 0, sizeof(*info)); + + rc = 0; + break; } out: diff --git a/xen/common/domctl.c b/xen/common/domctl.c index XXXXXXX..XXXXXXX 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -XXX,XX +XXX,XX @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_get_domain_state: - ret = xsm_get_domain_state(XSM_XS_PRIV, d); - if ( ret ) - break; - - copyback = 1; ret = get_domain_state(&op->u.get_domain_state, d, &op->domain); + if ( !ret ) + copyback = true; break; default: -- 2.39.5