Re: [PATCH 23/24] XSM: fold xsm_{,un}map_domain_irq() hooks

On 7/28/26 9:26 AM, Jan Beulich wrote:
> Like other resource management hooks they are (now) mainly different in
> "add resource" vs "remove resource". Hence like in other cases a single
> hook can easily serve both purposes, with minor tweaking of
> flask_map_domain_irq(). While adjusting that function, also defer the
> setting of local variables only needed in the "map" case.
> 
> Signed-off-by: Jan Beulich <[email protected]>
> 
> --- a/xen/arch/arm/domctl.c
> +++ b/xen/arch/arm/domctl.c
> @@ -100,7 +100,7 @@ long arch_do_domctl(struct xen_domctl *d
>            * done by the 2 hypercalls for consistency with other
>            * architectures.
>            */
> -        rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL);
> +        rc = xsm_map_domain_irq(XSM_HOOK, d, irq, NULL, true);
>           if ( rc )
>               return rc;
>   
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -2214,7 +2214,7 @@ int map_domain_pirq(
>           return 0;
>       }
>   
> -    ret = xsm_map_domain_irq(XSM_HOOK, d, irq, msi ? &msi->sbdf : NULL);
> +    ret = xsm_map_domain_irq(XSM_HOOK, d, irq, msi ? &msi->sbdf : NULL, true);
>       if ( ret )
>       {
>           dprintk(XENLOG_G_ERR, "dom%d: could not permit access to irq %d mapping to pirq %d\n",
> @@ -2441,8 +2441,8 @@ int unmap_domain_pirq(struct domain *d,
>        * domain.  Skip the XSM check since this is a Xen-initiated action.
>        */
>       if ( !d->is_dying )
> -        ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
> -                                   msi_desc ? &msi_desc->dev->sbdf : NULL);
> +        ret = xsm_map_domain_irq(XSM_HOOK, d, irq,
> +                                 msi_desc ? &msi_desc->dev->sbdf : NULL, false);
>   
>       if ( ret )
>           goto done;
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -470,7 +470,8 @@ static XSM_INLINE int xsm_map_domain_pir
>   #endif /* CONFIG_HAS_PIRQ */
>   
>   static XSM_INLINE int xsm_map_domain_irq(
> -    XSM_DEFAULT_ARG struct domain *d, int irq, const pci_sbdf_t *sbdf)
> +    XSM_DEFAULT_ARG struct domain *d, int irq, const pci_sbdf_t *sbdf,
> +    bool allow)
>   {
>       XSM_ASSERT_ACTION(XSM_HOOK);
>       return xsm_default_action(action, current->domain, d);
> @@ -490,13 +491,6 @@ static XSM_INLINE int xsm_unbind_pt_irq(
>       return xsm_default_action(action, current->domain, d);
>   }
>   
> -static XSM_INLINE int xsm_unmap_domain_irq(
> -    XSM_DEFAULT_ARG struct domain *d, int irq, const pci_sbdf_t *sbdf)
> -{
> -    XSM_ASSERT_ACTION(XSM_HOOK);
> -    return xsm_default_action(action, current->domain, d);
> -}
> -
>   static XSM_INLINE int xsm_irq_permission(
>       XSM_DEFAULT_ARG struct domain *d, int pirq, bool allow)
>   {
> --- a/xen/include/xsm/hooks.h
> +++ b/xen/include/xsm/hooks.h
> @@ -71,8 +71,7 @@ XSM_HOOK(int, schedop_shutdown, struct d
>   XSM_HOOK(int, map_domain_pirq, struct domain *, bool)
>   #endif
>   
> -XSM_HOOK(int, map_domain_irq, struct domain *, int, const pci_sbdf_t *)
> -XSM_HOOK(int, unmap_domain_irq, struct domain *, int, const pci_sbdf_t *)
> +XSM_HOOK(int, map_domain_irq, struct domain *, int, const pci_sbdf_t *, bool)
>   XSM_HOOK(int, bind_pt_irq, struct domain *, struct xen_domctl_bind_pt_irq *)
>   XSM_HOOK(int, unbind_pt_irq, struct domain *, struct xen_domctl_bind_pt_irq *)
>   
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1063,12 +1063,11 @@ static uint32_t flask_iommu_resource_use
>   }
>   
>   static int cf_check flask_map_domain_irq(
> -    struct domain *d, int irq, const pci_sbdf_t *sbdf)
> +    struct domain *d, int irq, const pci_sbdf_t *sbdf, bool access)
>   {
> -    uint32_t sid, dsid;
> +    uint32_t sid, dsid, dperm;
>       int rc = -EPERM;
>       struct avc_audit_data ad;
> -    uint32_t dperm = flask_iommu_resource_use_perm(d);
>   
>       if ( irq >= nr_static_irqs && sbdf )
>           rc = flask_map_domain_msi(d, irq, *sbdf, &sid, &ad);
> @@ -1078,33 +1077,16 @@ static int cf_check flask_map_domain_irq
>       if ( rc )
>           return rc;
>   
> -    dsid = domain_sid(d);
> -
> -    rc = avc_current_has_perm(sid, SECCLASS_RESOURCE, RESOURCE__ADD_IRQ, &ad);
> -    if ( rc )
> +    rc = avc_current_has_perm(sid, SECCLASS_RESOURCE,
> +                              access ? RESOURCE__ADD_IRQ : RESOURCE__REMOVE_IRQ,
> +                              &ad);
> +    if ( rc || access )

You've got this inverted. When `access == true`, i.e. the map operation, 
you are exiting here. When in fact the rest of the function is the 
final,and required perm check for map, but is now being enforced on the 
unmap operation where it is not needed.

v/r,
dps