Re: [PATCH] IOMMU: make domctl handler tolerate NULL domain

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2022 18:06, Andrew Cooper wrote:
> 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.wills@xxxxxxxxx>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> --- 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.

I find your wording ("plain broken" in particular) irritating, to put
it mildly. The change in behavior is that -EOPNOTSUPP may now be
returned for the gdbsx operation instead of -ENOSYS. And that's when
it would better have been -EOPNOTSUPP in the first place.

Apart from this I don't see a dependency on Jürgen's patch, so please
let me know if there's anything crucial I'm overlooking. Otherwise,
with two R-b, I'm intending to put in the patch irrespective of your
replies.

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

But that's not the case.

Jan