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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan