Re: [PATCH v3] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/04/2022 2:36 pm, Jan Beulich wrote:
> On 25.04.2022 14:37, Andrew Cooper wrote:
>> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
>> pointer.  One of several bugs here is known-but-compiled-out subops
>> falling
>> into the default chain and hitting unrelated logic.
>>
>> Remove the CONFIG_GDBSX ifdefary in arch_do_domctl() by implementing
>> gdbsx_domctl() and moving the logic across.
>>
>> As minor cleanup,
>>  * gdbsx_guest_mem_io() can become static
>>  * Remove opencoding of domain_vcpu() and %pd
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> Technically
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

I'll tweak the commit message now that the IOMMU fix has gone in.

> Yet as mentioned before, ...
>
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -816,71 +816,12 @@ long arch_do_domctl(
>>      }
>>  #endif
>>  
>> -#ifdef CONFIG_GDBSX
>>      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:
>> -    {
>> -        struct vcpu *v;
>> -
>> -        ret = -EBUSY;
>> -        if ( !d->controller_pause_count )
>> -            break;
>> -        ret = -EINVAL;
>> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
>> -             (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) ==
>> NULL )
>> -            break;
>> -        ret = vcpu_pause_by_systemcontroller(v);
>> -        break;
>> -    }
>> -
>>      case XEN_DOMCTL_gdbsx_unpausevcpu:
>> -    {
>> -        struct vcpu *v;
>> -
>> -        ret = -EBUSY;
>> -        if ( !d->controller_pause_count )
>> -            break;
>> -        ret = -EINVAL;
>> -        if ( domctl->u.gdbsx_pauseunp_vcpu.vcpu >= d->max_vcpus ||
>> -             (v = d->vcpu[domctl->u.gdbsx_pauseunp_vcpu.vcpu]) ==
>> NULL )
>> -            break;
>> -        ret = vcpu_unpause_by_systemcontroller(v);
>> -        if ( ret == -EINVAL )
>> -            printk(XENLOG_G_WARNING
>> -                   "WARN: d%d attempting to unpause %pv which is not
>> paused\n",
>> -                   currd->domain_id, v);
>> -        break;
>> -    }
>> -
>>      case XEN_DOMCTL_gdbsx_domstatus:
>> -    {
>> -        struct vcpu *v;
>> -
>> -        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;
>> +        ret = gdbsx_domctl(d, domctl, &copyback);
>>          break;
>> -    }
>> -#endif
>
> ... I'm not overly happy with the retaining of the case labels here
> (and the knock on effect it'll have for other subsystem domctl-s),

The crash in do_iommu_op() happened because things which weren't iommu
subops ended up in a function only expecting iommu subops, *because*
unrelated case labels got compiled out.

We've also had multiple XSAs elsewhere created by related "just chain
everything through default:" patterns.  The legacy MSR paths are still
especially guilty of doing this.


The case labels need to never ever get compiled out, even if their
subsystem has been.

So you have a choice between this patch, or a pattern of the form:

    case XEN_DOMCTL_gdbsx_domstatus:
        if ( !IS_ENABLED(CONFIG_GDBSX) )
        {
            ret = -Exxx;
            break;
        }
        ...

but the top level case labels need to stay one way or another.

For this patch, it moves a chunk of logic out of a fairly generic file
into its proper subsystem file, and a few extra case labels is a very
cheap price to pay.

~Andrew