Re: [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.04.2022 12:06, Andrew Cooper wrote:
> @@ -178,6 +179,71 @@ void domain_pause_for_debugger(void)
>          send_global_virq(VIRQ_DEBUGGER);
>  }
>  
> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool 
> *copyback)

Is there anything that requires "long" (and not just "int") here and ...

> +{
> +    struct vcpu *v;
> +    long ret = 0;

... here?

> +    switch ( domctl->cmd )
> +    {
> +    case XEN_DOMCTL_gdbsx_guestmemio:
> +        ret = gdbsx_guest_mem_io(d, &domctl->u.gdbsx_guest_memio);
> +        if ( !ret )
> +            *copyback = true;
> +        break;
> +
> +    case XEN_DOMCTL_gdbsx_pausevcpu:
> +        ret = -EBUSY;
> +        if ( !d->controller_pause_count )
> +            break;
> +        ret = -EINVAL;
> +        if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == 
> NULL )
> +            break;
> +        ret = vcpu_pause_by_systemcontroller(v);
> +        break;
> +
> +    case XEN_DOMCTL_gdbsx_unpausevcpu:
> +        ret = -EBUSY;
> +        if ( !d->controller_pause_count )
> +            break;
> +        ret = -EINVAL;
> +        if ( (v = domain_vcpu(d, domctl->u.gdbsx_pauseunp_vcpu.vcpu)) == 
> NULL )
> +            break;
> +        ret = vcpu_unpause_by_systemcontroller(v);
> +        if ( ret == -EINVAL )
> +            printk(XENLOG_G_WARNING
> +                   "WARN: %pd attempting to unpause %pv which is not 
> paused\n",
> +                   current->domain, v);
> +        break;
> +
> +    case XEN_DOMCTL_gdbsx_domstatus:
> +        domctl->u.gdbsx_domstatus.vcpu_id = -1;
> +        domctl->u.gdbsx_domstatus.paused = d->controller_pause_count > 0;
> +        if ( domctl->u.gdbsx_domstatus.paused )
> +        {
> +            for_each_vcpu ( d, v )
> +            {
> +                if ( v->arch.gdbsx_vcpu_event )
> +                {
> +                    domctl->u.gdbsx_domstatus.vcpu_id = v->vcpu_id;
> +                    domctl->u.gdbsx_domstatus.vcpu_ev =
> +                        v->arch.gdbsx_vcpu_event;
> +                    v->arch.gdbsx_vcpu_event = 0;
> +                    break;
> +                }
> +            }
> +        }
> +        *copyback = true;
> +        break;
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        ret = -ENOSYS;
> +    }

Just as a remark: It's never really clear to me whether we actually want
to permit omitting "break" in cases like this one. It always feels
slightly risky towards someone subsequently adding another case label
below here without adding the suddenly necessary "break". While for
sentinel code like this doing so may be okay, it would seem to me that
we might be better off not allowing omission of "break" anywhere.

> --- a/xen/arch/x86/include/asm/gdbsx.h
> +++ b/xen/arch/x86/include/asm/gdbsx.h
> @@ -2,18 +2,27 @@
>  #ifndef __X86_GDBX_H__
>  #define __X86_GDBX_H__
>  
> -#ifdef CONFIG_GDBSX
> +#include <xen/stdbool.h>
>  
>  struct domain;
> -struct xen_domctl_gdbsx_memio;
> +struct xen_domctl;
>  
> -int gdbsx_guest_mem_io(struct domain *d, struct xen_domctl_gdbsx_memio *iop);
> +#ifdef CONFIG_GDBSX
>  
>  void domain_pause_for_debugger(void);
>  
> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool 
> *copyback);
> +
>  #else
>  
> +#include <xen/errno.h>
> +
>  static inline void domain_pause_for_debugger(void) {}
>  
> +long gdbsx_domctl(struct domain *d, struct xen_domctl *domctl, bool 
> *copyback)

static inline?

Jan