[PATCH 0/2] x86/time: CF-clobber annotations

[PATCH 0/2] x86/time: CF-clobber annotations

[Jan Beulich]

1: use fake read_tsc()
2: add CF-clobber annotations

Jan

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

[Jan Beulich]

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
+ * 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 =
 {

[PATCH 2/2] x86/time: add CF-clobber annotations

[Jan Beulich]

With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
in place we can further arrange for ENDBR removal from the functions no
longer subject to indirect calls. Note that plt_tsc is left untouched,
for not holding any pointer eligible for ENDBR removal.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I did consider converting most of the plt_* to const (plt_hpet and
plt_pmtimer cannot be converted), but this would entail quite a few
further changes.

--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -375,7 +375,7 @@ static void cf_check resume_pit(struct p
     outb(0, PIT_CH2);     /* MSB of count */
 }
 
-static struct platform_timesource __initdata plt_pit =
+static struct platform_timesource __initdata_cf_clobber plt_pit =
 {
     .id = "pit",
     .name = "PIT",
@@ -483,7 +483,7 @@ static void cf_check resume_hpet(struct
     hpet_resume(NULL);
 }
 
-static struct platform_timesource __initdata plt_hpet =
+static struct platform_timesource __initdata_cf_clobber plt_hpet =
 {
     .id = "hpet",
     .name = "HPET",
@@ -528,7 +528,7 @@ static s64 __init cf_check init_pmtimer(
     return adjust_elapsed(rdtsc_ordered() - start, elapsed, target);
 }
 
-static struct platform_timesource __initdata plt_pmtimer =
+static struct platform_timesource __initdata_cf_clobber plt_pmtimer =
 {
     .id = "acpi",
     .name = "ACPI PM Timer",
@@ -683,7 +683,7 @@ static void cf_check resume_xen_timer(st
     write_atomic(&xen_timer_last, 0);
 }
 
-static struct platform_timesource __initdata plt_xen_timer =
+static struct platform_timesource __initdata_cf_clobber plt_xen_timer =
 {
     .id = "xen",
     .name = "XEN PV CLOCK",
@@ -780,7 +780,7 @@ static uint64_t cf_check read_hyperv_tim
     return hv_scale_tsc(tsc, scale, offset);
 }
 
-static struct platform_timesource __initdata plt_hyperv_timer =
+static struct platform_timesource __initdata_cf_clobber plt_hyperv_timer =
 {
     .id = "hyperv",
     .name = "HYPER-V REFERENCE TSC",

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

[Andrew Cooper]

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

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Andrew Cooper]

On 01/03/2022 11:06, Jan Beulich wrote:
> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
> in place we can further arrange for ENDBR removal from the functions no
> longer subject to indirect calls. Note that plt_tsc is left untouched,
> for not holding any pointer eligible for ENDBR removal.

I'd be tempted to include it, for consistency sake alone.

It is less likely to go wrong in the future if another hook is introduced.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

With the commit message, I'm not not certain if this is linked to the
previous patch.

Overall it looks fine, but I'd like to get clarity on this point.

> ---
> I did consider converting most of the plt_* to const (plt_hpet and
> plt_pmtimer cannot be converted), but this would entail quite a few
> further changes.

It's all initdata.  const is not terribly interesting, especially if it
is invasive and incomplete to do.

~Andrew

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

[Jan Beulich]

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

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Jan Beulich]

On 01.03.2022 14:18, Andrew Cooper wrote:
> On 01/03/2022 11:06, Jan Beulich wrote:
>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
>> in place we can further arrange for ENDBR removal from the functions no
>> longer subject to indirect calls. Note that plt_tsc is left untouched,
>> for not holding any pointer eligible for ENDBR removal.
> 
> I'd be tempted to include it, for consistency sake alone.
> 
> It is less likely to go wrong in the future if another hook is introduced.

Can do, sure.

>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> With the commit message, I'm not not certain if this is linked to the
> previous patch.
> 
> Overall it looks fine, but I'd like to get clarity on this point.

Converting read_tsc() was discussed (with Roger) earlier on, so I'd
view this as a separate change. As said in reply to your comments on
the 1st patch, how exactly read_tsc() looks like is orthogonal to
the changes here at least as long as it doesn't live in .text.

Jan

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Andrew Cooper]

On 01/03/2022 14:18, Jan Beulich wrote:
> On 01.03.2022 14:18, Andrew Cooper wrote:
>> On 01/03/2022 11:06, Jan Beulich wrote:
>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
>>> in place we can further arrange for ENDBR removal from the functions no
>>> longer subject to indirect calls. Note that plt_tsc is left untouched,
>>> for not holding any pointer eligible for ENDBR removal.
>> I'd be tempted to include it, for consistency sake alone.
>>
>> It is less likely to go wrong in the future if another hook is introduced.
> Can do, sure.
>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> With the commit message, I'm not not certain if this is linked to the
>> previous patch.
>>
>> Overall it looks fine, but I'd like to get clarity on this point.
> Converting read_tsc() was discussed (with Roger) earlier on, so I'd
> view this as a separate change. As said in reply to your comments on
> the 1st patch, how exactly read_tsc() looks like is orthogonal to
> the changes here at least as long as it doesn't live in .text.

Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Andrew Cooper]

On 01/03/2022 14:24, Andrew Cooper wrote:
> On 01/03/2022 14:18, Jan Beulich wrote:
>> On 01.03.2022 14:18, Andrew Cooper wrote:
>>> On 01/03/2022 11:06, Jan Beulich wrote:
>>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
>>>> in place we can further arrange for ENDBR removal from the functions no
>>>> longer subject to indirect calls. Note that plt_tsc is left untouched,
>>>> for not holding any pointer eligible for ENDBR removal.
>>> I'd be tempted to include it, for consistency sake alone.
>>>
>>> It is less likely to go wrong in the future if another hook is introduced.
>> Can do, sure.
>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> With the commit message, I'm not not certain if this is linked to the
>>> previous patch.
>>>
>>> Overall it looks fine, but I'd like to get clarity on this point.
>> Converting read_tsc() was discussed (with Roger) earlier on, so I'd
>> view this as a separate change. As said in reply to your comments on
>> the 1st patch, how exactly read_tsc() looks like is orthogonal to
>> the changes here at least as long as it doesn't live in .text.
> Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Actually, you probably want to move plt_src into __ro_after_init along
with this change.

~Andrew

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

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

[Jan Beulich]

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

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Jan Beulich]

On 01.03.2022 15:24, Andrew Cooper wrote:
> On 01/03/2022 14:18, Jan Beulich wrote:
>> On 01.03.2022 14:18, Andrew Cooper wrote:
>>> On 01/03/2022 11:06, Jan Beulich wrote:
>>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
>>>> in place we can further arrange for ENDBR removal from the functions no
>>>> longer subject to indirect calls. Note that plt_tsc is left untouched,
>>>> for not holding any pointer eligible for ENDBR removal.
>>> I'd be tempted to include it, for consistency sake alone.
>>>
>>> It is less likely to go wrong in the future if another hook is introduced.
>> Can do, sure.
>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> With the commit message, I'm not not certain if this is linked to the
>>> previous patch.
>>>
>>> Overall it looks fine, but I'd like to get clarity on this point.
>> Converting read_tsc() was discussed (with Roger) earlier on, so I'd
>> view this as a separate change. As said in reply to your comments on
>> the 1st patch, how exactly read_tsc() looks like is orthogonal to
>> the changes here at least as long as it doesn't live in .text.
> 
> Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks. I'll take it this includes annotating plt_tsc as well, just
as a precautionary measure (and as you did suggest; still visible in
context above).

Jan

Re: [PATCH 2/2] x86/time: add CF-clobber annotations

[Jan Beulich]

On 01.03.2022 15:35, Andrew Cooper wrote:
> On 01/03/2022 14:24, Andrew Cooper wrote:
>> On 01/03/2022 14:18, Jan Beulich wrote:
>>> On 01.03.2022 14:18, Andrew Cooper wrote:
>>>> On 01/03/2022 11:06, Jan Beulich wrote:
>>>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall")
>>>>> in place we can further arrange for ENDBR removal from the functions no
>>>>> longer subject to indirect calls. Note that plt_tsc is left untouched,
>>>>> for not holding any pointer eligible for ENDBR removal.
>>>> I'd be tempted to include it, for consistency sake alone.
>>>>
>>>> It is less likely to go wrong in the future if another hook is introduced.
>>> Can do, sure.
>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> With the commit message, I'm not not certain if this is linked to the
>>>> previous patch.
>>>>
>>>> Overall it looks fine, but I'd like to get clarity on this point.
>>> Converting read_tsc() was discussed (with Roger) earlier on, so I'd
>>> view this as a separate change. As said in reply to your comments on
>>> the 1st patch, how exactly read_tsc() looks like is orthogonal to
>>> the changes here at least as long as it doesn't live in .text.
>> Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Actually, you probably want to move plt_src into __ro_after_init along
> with this change.

I'd view this as an independent change. Perhaps it would make for a
better change if we went through and converted from __read_mostly for
a bunch of items all in one go.

Jan