On 01.03.2022 14:07, Andrew Cooper wrote:
> On 01/03/2022 11:05, Jan Beulich wrote:
>> Go a step further than bed9ae54df44 ("x86/time: switch platform timer
>> hooks to altcall") did and eliminate the "real" read_tsc() altogether:
>> It's not used except in pointer comparisons, and hence it looks overall
>> more safe to simply poison plt_tsc's read_counter hook.
>>
>> Signed-off-by: Jan Beulich <[email protected]>
>> ---
>> I wasn't really sure whether it would be better to use simply void * for
>> the type of the expression, resulting in an undesirable data -> function
>> pointer conversion, but making it impossible to mistakenly try and call
>> the (fake) function directly.
>>
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -585,10 +585,12 @@ static s64 __init cf_check init_tsc(stru
>> return ret;
>> }
>>
>> -static uint64_t __init cf_check read_tsc(void)
>> -{
>> - return rdtsc_ordered();
>> -}
>> +/*
>> + * plt_tsc's read_counter hook is (and should not be) invoked via the struct
>
> Either "is not (and should not be)" or "is (and should) not be".
Oh, of course.
>> + * field. To avoid carrying an unused, indirectly reachable function, poison
>> + * the field with an easily identifiable non-canonical pointer.
>> + */
>> +#define read_tsc ((uint64_t(*)(void))0x75C75C75C75C75C0ul)
>
> How about using ZERO_BLOCK_PTR? The hex constant 0xBAD0... is more
> easily recognisable, and any poisoned pointer will do.
Well, I specifically wanted to _not_ re-use any of the constants we
already use.
> That said... what's wrong a plain NULL? I can't see any need for a
> magic constant here.
Are you fancying an XSA for a call through NULL in PV guest context?
> Overall, I think this patch should be merged with the subsequent one,
> because in isolation it is slightly dubious. read_tsc() is one of the
> few functions which is of no interest to an attacker, architecturally
> (because it's just rdtsc) or speculatively (because it is dispatch
> serialising).
What code would appear to live at read_tsc()'s address at the time an
attacker can come into play is unknown, given it's __init.
> This change is only (AFAICT) to allow the use of cf_clobber later.
Not exactly. The subsequent patch specifically does not touch plt_tsc.
And if it did, it would have no effect with read_tsc() living in
.init.text.
Jan
