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.
If you think it's wrong, it in a separate change and don't block this fix.
~Andrew
