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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew