Re: [PATCH 1/2] x86/time: use fake read_tsc()

[Jan Beulich <jbeulich@xxxxxxxx>]

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 <jbeulich@xxxxxxxx>
>> ---
>> 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