| Message ID | f4f0e92c-9586-e021-6ad5-718628f88fcd@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/time: CF-clobber annotations | expand |
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@suse.com> > --- > 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
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@suse.com> >> --- >> 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
On 01/03/2022 14:14, Jan Beulich wrote: > On 01.03.2022 14:07, Andrew Cooper wrote: >> On 01/03/2022 11:05, Jan Beulich wrote: >> 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? Why do you think that a risk? Only non-NULL function pointers are followed, and altcall resolves safely if the pointer is still NULL. And on that subject, don't we not hit the altcall's BUG_ON() for exceeding disp32? ~Andrew
On 01/03/2022 14:39, Andrew Cooper wrote: > On 01/03/2022 14:14, Jan Beulich wrote: >> On 01.03.2022 14:07, Andrew Cooper wrote: >>> On 01/03/2022 11:05, Jan Beulich wrote: >>> 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? > Why do you think that a risk? Only non-NULL function pointers are > followed, and altcall resolves safely if the pointer is still NULL. > > And on that subject, don't we not hit the altcall's BUG_ON() for > exceeding disp32? don't we hit* ~Andrew
On 01.03.2022 15:39, Andrew Cooper wrote: > On 01/03/2022 14:14, Jan Beulich wrote: >> On 01.03.2022 14:07, Andrew Cooper wrote: >>> On 01/03/2022 11:05, Jan Beulich wrote: >>> 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? > > Why do you think that a risk? Only non-NULL function pointers are > followed, and altcall resolves safely if the pointer is still NULL. > > And on that subject, don't we not hit the altcall's BUG_ON() for > exceeding disp32? There's no altcall involved here. As said in earlier contexts, altcall patching comes to early to cover plt_tsc use. Hence the only concern is a non-altacll-ed use of the pointer. Jan
--- 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 + * 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) static struct platform_timesource __initdata plt_tsc = {
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@suse.com> --- 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.