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".
> + * 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.
That said... what's wrong a plain NULL? I can't see any need for a
magic constant here.
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).
This change is only (AFAICT) to allow the use of cf_clobber later.
~Andrew
