[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/2] x86/time: use fake read_tsc()
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |