[v1] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

[PATCH RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

Posted by Andrew Cooper 2 years ago

When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
pointer.  It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall
into the default case when compiled out.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Wei Liu <wl@xen.org>
CC: Julien Grall <julien@xen.org>
CC: Juergen Gross <jgross@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>

RFC, because this has implications across the codebase.  The tl;dr is that
case FOO:'s shouldn't be compiled out; we still know what the subops are, even
when the functionality is compiled out.

There are several ways to express this.  Alternatives would be:

    case XEN_DOMCTL_gdbsx_guestmemio:
        if ( !IS_ENABLED(CONFIG_GDBSX) )
        {
            rc = -EOPNOTSUPP;
            break;
        }
        ...;

but given my debugger series creating gdbsx.c, I was also considering:

    case XEN_DOMCTL_gdbsx_guestmemio:
    case XEN_DOMCTL_gdbsx_pausevcpu:
    case XEN_DOMCTL_gdbsx_unpausevcpu:
    case XEN_DOMCTL_gdbsx_domstatus:
        rc = gdbsx_do_domctl(d, iop);
        break;

when I can rework the callers of domain_pause_for_debugger() slightly, at
which point we can conditionally compile the gdbsx variables out struct
domain/vcpu, which wouldn't be compatible with the first suggestion.

Thoughts?
---
 xen/arch/x86/domctl.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a6aae500a30b..1faa5a49ff3c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -890,7 +890,14 @@ long arch_do_domctl(
         copyback = true;
         break;
     }
-#endif
+#else /* CONFIG_GDBSX */
+    case XEN_DOMCTL_gdbsx_guestmemio:
+    case XEN_DOMCTL_gdbsx_pausevcpu:
+    case XEN_DOMCTL_gdbsx_unpausevcpu:
+    case XEN_DOMCTL_gdbsx_domstatus:
+        rc = -EOPNOTSUPP;
+        break;
+#endif /* CONFIG_GDBSX */
 
     case XEN_DOMCTL_setvcpuextstate:
     case XEN_DOMCTL_getvcpuextstate:
-- 
2.11.0

Re: [PATCH RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

Posted by Juergen Gross 2 years ago

On 20.04.22 17:56, Andrew Cooper wrote:
> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
> pointer.  It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall
> into the default case when compiled out.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: George Dunlap <George.Dunlap@eu.citrix.com>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Wei Liu <wl@xen.org>
> CC: Julien Grall <julien@xen.org>
> CC: Juergen Gross <jgross@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> RFC, because this has implications across the codebase.  The tl;dr is that
> case FOO:'s shouldn't be compiled out; we still know what the subops are, even
> when the functionality is compiled out.
> 
> There are several ways to express this.  Alternatives would be:
> 
>      case XEN_DOMCTL_gdbsx_guestmemio:
>          if ( !IS_ENABLED(CONFIG_GDBSX) )
>          {
>              rc = -EOPNOTSUPP;
>              break;
>          }
>          ...;
> 
> but given my debugger series creating gdbsx.c, I was also considering:
> 
>      case XEN_DOMCTL_gdbsx_guestmemio:
>      case XEN_DOMCTL_gdbsx_pausevcpu:
>      case XEN_DOMCTL_gdbsx_unpausevcpu:
>      case XEN_DOMCTL_gdbsx_domstatus:
>          rc = gdbsx_do_domctl(d, iop);
>          break;

I'd go this route.


Juergen

Re: [PATCH RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

Posted by Jan Beulich 2 years ago

On 20.04.2022 18:03, Juergen Gross wrote:
> On 20.04.22 17:56, Andrew Cooper wrote:
>> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
>> pointer.  It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall
>> into the default case when compiled out.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Wei Liu <wl@xen.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Juergen Gross <jgross@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> RFC, because this has implications across the codebase.  The tl;dr is that
>> case FOO:'s shouldn't be compiled out; we still know what the subops are, even
>> when the functionality is compiled out.
>>
>> There are several ways to express this.  Alternatives would be:
>>
>>      case XEN_DOMCTL_gdbsx_guestmemio:
>>          if ( !IS_ENABLED(CONFIG_GDBSX) )
>>          {
>>              rc = -EOPNOTSUPP;
>>              break;
>>          }
>>          ...;
>>
>> but given my debugger series creating gdbsx.c, I was also considering:
>>
>>      case XEN_DOMCTL_gdbsx_guestmemio:
>>      case XEN_DOMCTL_gdbsx_pausevcpu:
>>      case XEN_DOMCTL_gdbsx_unpausevcpu:
>>      case XEN_DOMCTL_gdbsx_domstatus:
>>          rc = gdbsx_do_domctl(d, iop);
>>          break;
> 
> I'd go this route.

+1 if we already start enumerating sub-system domctl-s (as proposed
for the IOMMU ones as well).

Jan

Re: [PATCH RFC] x86: Fix XEN_DOMCTL_gdbsx_guestmemio crash

Posted by Andrew Cooper 2 years ago

On 21/04/2022 10:26, Jan Beulich wrote:
> On 20.04.2022 18:03, Juergen Gross wrote:
>> On 20.04.22 17:56, Andrew Cooper wrote:
>>> When CONFIG_GDBSX is compiled out, iommu_do_domctl() falls over a NULL
>>> pointer.  It isn't really correct for processing of XEN_DOMCTL_gdbsx_* to fall
>>> into the default case when compiled out.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: George Dunlap <George.Dunlap@eu.citrix.com>
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Wei Liu <wl@xen.org>
>>> CC: Julien Grall <julien@xen.org>
>>> CC: Juergen Gross <jgross@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>>>
>>> RFC, because this has implications across the codebase.  The tl;dr is that
>>> case FOO:'s shouldn't be compiled out; we still know what the subops are, even
>>> when the functionality is compiled out.
>>>
>>> There are several ways to express this.  Alternatives would be:
>>>
>>>      case XEN_DOMCTL_gdbsx_guestmemio:
>>>          if ( !IS_ENABLED(CONFIG_GDBSX) )
>>>          {
>>>              rc = -EOPNOTSUPP;
>>>              break;
>>>          }
>>>          ...;
>>>
>>> but given my debugger series creating gdbsx.c, I was also considering:
>>>
>>>      case XEN_DOMCTL_gdbsx_guestmemio:
>>>      case XEN_DOMCTL_gdbsx_pausevcpu:
>>>      case XEN_DOMCTL_gdbsx_unpausevcpu:
>>>      case XEN_DOMCTL_gdbsx_domstatus:
>>>          rc = gdbsx_do_domctl(d, iop);
>>>          break;
>> I'd go this route.
> +1 if we already start enumerating sub-system domctl-s (as proposed
> for the IOMMU ones as well).

Ok, so that seems like general agreement for "we shouldn't compile out
case statements" which is something we should apply across the codebase
going forward.

For gdbsx_do_domctl() specifically, that's going to want to wait until
"[PATCH v5 0/6] Clean up common/arch split for debugger.h" has gone in,
but I don't anticipate any issues there.

~Andrew