Re: [XEN PATCH v12 4/7] x86/domctl: Add hypercall to set the access of x86 gsi

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 26 Jul 2024, Chen, Jiqian wrote:
> On 2024/7/23 06:10, Stefano Stabellini wrote:
> > On Mon, 8 Jul 2024, Jiqian Chen wrote:
> >> Some type of domains don't have PIRQs, like PVH, it doesn't do
> >> PHYSDEVOP_map_pirq for each gsi. When passthrough a device
> >> to guest base on PVH dom0, callstack
> >> pci_add_dm_done->XEN_DOMCTL_irq_permission will fail at function
> >> domain_pirq_to_irq, because PVH has no mapping of gsi, pirq and
> >> irq on Xen side.
> >> What's more, current hypercall XEN_DOMCTL_irq_permission requires
> >> passing in pirq to set the access of irq, it is not suitable for
> >> dom0 that doesn't have PIRQs.
> >>
> >> So, add a new hypercall XEN_DOMCTL_gsi_permission to grant/deny
> >> the permission of irq(translate from x86 gsi) to dumU when dom0
> >> has no PIRQs.
> >>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> >> Signed-off-by: Jiqian Chen <Jiqian.Chen@xxxxxxx>
> >> ---
> >> CC: Daniel P . Smith <dpsmith@xxxxxxxxxxxxxxxxxxxx>
> >> Remaining comment @Daniel P . Smith:
> >> +        ret = -EPERM;
> >> +        if ( !irq_access_permitted(currd, irq) ||
> >> +             xsm_irq_permission(XSM_HOOK, d, irq, access_flag) )
> >> +            goto gsi_permission_out;
> >> Is it okay to issue the XSM check using the translated value, 
> >> not the one that was originally passed into the hypercall?
> >> ---
> >>  xen/arch/x86/domctl.c              | 32 ++++++++++++++++++++++++++++++
> >>  xen/arch/x86/include/asm/io_apic.h |  2 ++
> >>  xen/arch/x86/io_apic.c             | 17 ++++++++++++++++
> >>  xen/arch/x86/mpparse.c             |  5 ++---
> >>  xen/include/public/domctl.h        |  9 +++++++++
> >>  xen/xsm/flask/hooks.c              |  1 +
> >>  6 files changed, 63 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> >> index 9190e11faaa3..4e9e4c4cfed3 100644
> >> --- a/xen/arch/x86/domctl.c
> >> +++ b/xen/arch/x86/domctl.c
> >> @@ -36,6 +36,7 @@
> >>  #include <asm/xstate.h>
> >>  #include <asm/psr.h>
> >>  #include <asm/cpu-policy.h>
> >> +#include <asm/io_apic.h>
> >>  
> >>  static int update_domain_cpu_policy(struct domain *d,
> >>                                      xen_domctl_cpu_policy_t *xdpc)
> >> @@ -237,6 +238,37 @@ long arch_do_domctl(
> >>          break;
> >>      }
> >>  
> >> +    case XEN_DOMCTL_gsi_permission:
> >> +    {
> >> +        int irq;
> >> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
> >> +        uint8_t access_flag = domctl->u.gsi_permission.access_flag;
> >> +
> >> +        /* Check all bits and pads are zero except lowest bit */
> >> +        ret = -EINVAL;
> >> +        if ( access_flag & ( ~XEN_DOMCTL_GSI_PERMISSION_MASK ) )
> >> +            goto gsi_permission_out;
> >> +        for ( i = 0; i < ARRAY_SIZE(domctl->u.gsi_permission.pad); ++i )
> >> +            if ( domctl->u.gsi_permission.pad[i] )
> >> +                goto gsi_permission_out;
> >> +
> >> +        if ( gsi > highest_gsi() || (irq = gsi_2_irq(gsi)) <= 0 )
> > 
> > gsi is unsigned int but it is passed to gsi_2_irq which takes an int as
> > parameter. If gsi >= INT32_MAX we have a problem. I think we should
> > explicitly check for the possible overflow and return error in that
> > case.
> But here has checked "gsi > highest_gsi()", can highesi_gsi() return a gsi >= 
> INT32_MAX?

In practice it is impossible but in theory it could. But then I looked
at the implementation of highest_gsi() and gsi_end actually a signed
int. So I think this is OK:

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>