Re: [PATCH v2] xen: fix XEN_DOMCTL_gdbsx_guestmemio crash

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 19/04/2022 11:51, Juergen Gross wrote:
> On 19.04.22 12:40, Andrew Cooper wrote:
>> On 19/04/2022 11:18, Juergen Gross wrote:
>>> A hypervisor built without CONFIG_GDBSX will crash in case the
>>> XEN_DOMCTL_gdbsx_guestmemio domctl is being called, as the call will
>>> end up in iommu_do_domctl() with d == NULL:
>>>
>>> (XEN) CPU:    6
>>> (XEN) RIP:    e008:[<ffff82d040269984>] iommu_do_domctl+0x4/0x30
>>> (XEN) RFLAGS: 0000000000010202   CONTEXT: hypervisor (d0v0)
>>> (XEN) rax: 00000000000003e8   rbx: ffff830856277ef8   rcx:
>>> ffff830856277fff
>>> ...
>>> (XEN) Xen call trace:
>>> (XEN)    [<ffff82d040269984>] R iommu_do_domctl+0x4/0x30
>>> (XEN)    [<ffff82d04035cd5f>] S arch_do_domctl+0x7f/0x2330
>>> (XEN)    [<ffff82d040239e46>] S do_domctl+0xe56/0x1930
>>> (XEN)    [<ffff82d040238ff0>] S do_domctl+0/0x1930
>>> (XEN)    [<ffff82d0402f8c59>] S pv_hypercall+0x99/0x110
>>> (XEN)    [<ffff82d0402f5161>] S
>>> arch/x86/pv/domain.c#_toggle_guest_pt+0x11/0x90
>>> (XEN)    [<ffff82d040366288>] S lstar_enter+0x128/0x130
>>> (XEN)
>>> (XEN) Pagetable walk from 0000000000000144:
>>> (XEN)  L4[0x000] = 0000000000000000 ffffffffffffffff
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 6:
>>> (XEN) FATAL PAGE FAULT
>>> (XEN) [error_code=0000]
>>> (XEN) Faulting linear address: 0000000000000144
>>>
>>> Fix this issue by modifying the interface of gdbsx_guest_mem_io() to
>>> take the already known domain pointer instead of the domid.
>>
>> There is some explanation missing here.  The adjustments you make are
>> within CONFIG_GDBSX, with the exception of the final hunk.
>
> Yeah, and this is the one really fixing the issue, while the
> other hunks are needed to cope with the way the problem is
> fixed.

In which case the salient point needed in the commit message is "reject
the use of XEN_DOMCTL_gdbsx_guestmemio without a valid domain".

I'd go so far as to say that that ought to be a oneliner fix, which also
discusses why it's safe now (it didn't used to be IIRC), and the cleanup
in a separate patch.

This also reminds me that there's a pile of almost complete series of
debugger/gdbsx/traps cleanup in need of pushing over the line.

>
>> The actual bug is that non-IOMMU subops end up in iommu_do_domctl(), so
>> while this is good cleanup to gdbsx_guest_mem_io() it, along with Jan's
>> adjustment to iommu_do_domctl(), are not suitable fixes to the crash as
>> reported.
>
> The same way non-arch subops might end up in arch_do_domctl().
>
> What would be the right way to fix that in your opinion?
>
> IMO any subop handler called under the default label of a switch() should
> be able to handle unknown subops. This is done for iommu_do_domctl() in
> Jan's patch by not dereferencing d unconditionally.

The problem isn't (specifically) how they're chained; it's the name. 
arch_do_domctl() is clearly a dumping ground for arbitrary subops. 
iommu_do_domctl() is clearly not.

The bug was ultimately chaining iommu_do_domctl() in a default, which is
why it *was* reasonable for "use is_iommu_enabled() where
appropriate..." to assume that only iommu subops would reach the function.

iommu_do_domctl() either needs re-chaining under several case xxx:, or
it needs renaming to something generic like arch_do_domctl2().

Nothing else is going to leave the code in a position where it's harder
to make mistakes like this.

~Andrew