Re: [PATCH v2] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 25/04/2022 12:04, Jan Beulich wrote:
> 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?

Consistency with its caller.

Although I can't actually see a good reason for arch_do_domctl() to use
long's either, and that would at least mean we've only got one place
doing extension of ret.

>
>> +    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.

That was an oversight.  I'd intended to include one.

>> --- 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?

Oops yes.  I clearly need more coffee.

~Andrew