On 19/04/2022 16:52, Juergen Gross wrote:
> On 19.04.22 17:48, Andrew Cooper wrote:
>> On 19/04/2022 10:39, Jan Beulich wrote:
>>> Besides the reporter's issue of hitting a NULL deref when
>>> !CONFIG_GDBSX,
>>> XEN_DOMCTL_test_assign_device can legitimately end up having NULL
>>> passed
>>> here, when the domctl was passed DOMID_INVALID.
>>>
>>> Fixes: 71e617a6b8f6 ("use is_iommu_enabled() where appropriate...")
>>> Reported-by: Cheyenne Wills <cheyenne.wi...@gmail.com>
>>> Signed-off-by: Jan Beulich <jbeul...@suse.com>
>>>
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -558,7 +558,7 @@ int iommu_do_domctl(
>>> {
>>> int ret = -ENODEV;
>>> - if ( !is_iommu_enabled(d) )
>>> + if ( !(d ? is_iommu_enabled(d) : iommu_enabled) )
>>> return -EOPNOTSUPP;
>>
>> Having spent the better part of a day debugging this mess, this patch is
>> plain broken.
>>
>> It depends on Juergen's "xen/iommu: cleanup iommu related domctl
>> handling" patch, because otherwise it erroneously fails non-IOMMU
>> subops.
>
> Which is not a real problem, as it is being called after all other
> subops didn't match.
It is a real problem even now, because it is bogus for the host or
domain's IOMMU status to have any alteration to the
XEN_DOMCTL_gdbsx_guestmemio path. The root cause of this bug is the
existing XEN_DOMCTL_gdbsx_guestmemio case being compiled out in the
!GDBSX case.
It would be a more obvious problem if there were calls chained after
iommu_do_domctl() in the arch_domctl() default: blocks, because then it
wouldn't only be compiled-out functionality which hit this check.
~Andrew
