Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2023 19:02, Andrew Cooper wrote:
> On 30/08/2023 5:13 pm, Jan Beulich wrote:
>> On 30.08.2023 17:28, Andrew Cooper wrote:
>>> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>>>> On 30.08.2023 16:35, Andrew Cooper wrote:
>>>>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>>>>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>>>>>  #endif
>>>>>>>      flags = c(flags);
>>>>>>>  
>>>>>>> +    if ( !compat )
>>>>>>> +    {
>>>>>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>>>>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>>>>>> +            return -EINVAL;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      if ( is_pv_domain(d) )
>>>>>>>      {
>>>>>>> +        /*
>>>>>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>>>>>> +         * subset of dr7, and dr4 was unused.
>>>>>>> +         *
>>>>>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored 
>>>>>>> for
>>>>>>> +         * backwards compatibility, and dr7 emulation is handled
>>>>>>> +         * internally.
>>>>>>> +         */
>>>>>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>>>>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>>>>> Don't you mean __addr_ok() here, i.e. not including the
>>>>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>>>>> sizeof(long), but that question resolves itself with using the other
>>>>>> macro.)
>>>>> For now, I'm simply moving a check from set_debugreg() earlier in
>>>>> arch_set_info_guest().
>>>>>
>>>>> I think it would be beneficial to keep that change independent.
>>>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>>>> you duplicate it here, and duplicating questionable code is, well,
>>>> questionable.
>>> It can't be removed in set_debugreg() because that's used in other paths
>>> too.
>> Sure, I understand that.
>>
>>> And the error from set_debugreg() can't fail arch_set_info_guest()
>>> because that introduces a failure after mutation of the vCPU state.
>>>
>>> This isn't a fastpath.  It's used approximately once per vCPU lifetime.
>> But fast or not isn't the point here.
> 
> No.  The point is no change from the existing code.

Having thought about it over night: It's not nice but okay to duplicate
the bogus check here, but then please say that and why you do so in the
description. With that suitably added
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> If you think it's wrong, it in a separate change and don't block this fix.

I would like to ask you to think about the opposite case occurring: I'm
pretty sure you wouldn't let me get away. Either - like so often - you'd
simply not reply anymore at a certain point, or - like here - you'd
expect me to adjust to your expectations.

Jan