Suitable compiler options are passed only when the actual feature
(XEN_IBT) is enabled, not when merely the compiler capability was found
to be available.
Fixes: 12e3410e071e ("x86/altcall: Check and optimise altcall targets")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Furthermore, is "Optimised away ..." really appropriate in what
37ed5da851b8 ("x86/altcall: Optimise away endbr64 instruction where
possible") added? If this really was an optimization (rather than
hardening), shouldn't we purge ENDBR also when !cpu_has_xen_ibt, and
then ideally all of them? Whereas if this is mainly about hardening,
wouldn't the message better say "Purged" or "Clobbered"?
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -295,7 +295,7 @@ static void init_or_livepatch _apply_alt
* marginal perf improvement which saves on instruction
* decode bandwidth.
*/
- if ( IS_ENABLED(CONFIG_HAS_CC_CET_IBT) )
+ if ( IS_ENABLED(CONFIG_XEN_IBT) )
{
if ( is_endbr64(dest) )
dest += ENDBR64_LEN;On 01/03/2022 11:36, Jan Beulich wrote:
> Suitable compiler options are passed only when the actual feature
> (XEN_IBT) is enabled, not when merely the compiler capability was found
> to be available.
>
> Fixes: 12e3410e071e ("x86/altcall: Check and optimise altcall targets")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Hmm yes. This is fallout from separating CONFIG_HAS_CC_CET_IBT and
CONFIG_XEN_IBT.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> Furthermore, is "Optimised away ..." really appropriate in what
> 37ed5da851b8 ("x86/altcall: Optimise away endbr64 instruction where
> possible") added? If this really was an optimization (rather than
> hardening), shouldn't we purge ENDBR also when !cpu_has_xen_ibt, and
> then ideally all of them? Whereas if this is mainly about hardening,
> wouldn't the message better say "Purged" or "Clobbered"?
I did have an RFC about this. Turning ENDBR into NOP4 matters, on a
CET-IBT-active system, to restrict the available options an attacker has
when they have already managed to hijack a function pointer (i.e.
already got a partial write gadget).
From that point of view, it is hardening.
The first version of this logic was trying to use UD1 in the same way as
Linux does, to harden non-CET builds too, but that does depend on having
objtool so all direct branches can have their targets updated to miss
the UD1 instruction.
~Andrew
P.S. I'd still like to have "away %u of %u endbr64's". It occurs to me
that now we have check-endbr64.sh, we could `wc -l` the $VALID file and
re-link this back in, but then we couldn't check the final objects.
On 01.03.2022 13:48, Andrew Cooper wrote:
> On 01/03/2022 11:36, Jan Beulich wrote:
>> Suitable compiler options are passed only when the actual feature
>> (XEN_IBT) is enabled, not when merely the compiler capability was found
>> to be available.
>>
>> Fixes: 12e3410e071e ("x86/altcall: Check and optimise altcall targets")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> Hmm yes. This is fallout from separating CONFIG_HAS_CC_CET_IBT and
> CONFIG_XEN_IBT.
>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks.
>> ---
>> Furthermore, is "Optimised away ..." really appropriate in what
>> 37ed5da851b8 ("x86/altcall: Optimise away endbr64 instruction where
>> possible") added? If this really was an optimization (rather than
>> hardening), shouldn't we purge ENDBR also when !cpu_has_xen_ibt, and
>> then ideally all of them? Whereas if this is mainly about hardening,
>> wouldn't the message better say "Purged" or "Clobbered"?
>
> I did have an RFC about this. Turning ENDBR into NOP4 matters, on a
> CET-IBT-active system, to restrict the available options an attacker has
> when they have already managed to hijack a function pointer (i.e.
> already got a partial write gadget).
>
> From that point of view, it is hardening.
But then you don't say whether there's any optimization aspect here.
My question was really about the wording of that log message.
> The first version of this logic was trying to use UD1 in the same way as
> Linux does, to harden non-CET builds too, but that does depend on having
> objtool so all direct branches can have their targets updated to miss
> the UD1 instruction.
I think it would be possible (but likely cumbersome) to arrange for
this also without objtool.
> P.S. I'd still like to have "away %u of %u endbr64's". It occurs to me
> that now we have check-endbr64.sh, we could `wc -l` the $VALID file and
> re-link this back in, but then we couldn't check the final objects.
I was thinking about this wish of yours as well; I simply didn't know how
important you view it to have this information. Not being able to check
the final objects is not an issue: If the data is available after the 2nd
linking pass, contents of .text isn't going to change anymore. All
addresses are the same for the 2nd and 3rd linking passes. Hence if the
checking was inserted between these two passes, the value of interest
could be propagated suitably.
Jan
On 01/03/2022 13:34, Jan Beulich wrote:
> On 01.03.2022 13:48, Andrew Cooper wrote:
>> On 01/03/2022 11:36, Jan Beulich wrote:
>>> Suitable compiler options are passed only when the actual feature
>>> (XEN_IBT) is enabled, not when merely the compiler capability was found
>>> to be available.
>>>
>>> Fixes: 12e3410e071e ("x86/altcall: Check and optimise altcall targets")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Hmm yes. This is fallout from separating CONFIG_HAS_CC_CET_IBT and
>> CONFIG_XEN_IBT.
>>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Thanks.
>
>>> ---
>>> Furthermore, is "Optimised away ..." really appropriate in what
>>> 37ed5da851b8 ("x86/altcall: Optimise away endbr64 instruction where
>>> possible") added? If this really was an optimization (rather than
>>> hardening), shouldn't we purge ENDBR also when !cpu_has_xen_ibt, and
>>> then ideally all of them? Whereas if this is mainly about hardening,
>>> wouldn't the message better say "Purged" or "Clobbered"?
>> I did have an RFC about this. Turning ENDBR into NOP4 matters, on a
>> CET-IBT-active system, to restrict the available options an attacker has
>> when they have already managed to hijack a function pointer (i.e.
>> already got a partial write gadget).
>>
>> From that point of view, it is hardening.
> But then you don't say whether there's any optimization aspect here.
> My question was really about the wording of that log message.
The optimisation aspect is direct branch target +4, because that is what
saves on decode bandwidth.
>
>> The first version of this logic was trying to use UD1 in the same way as
>> Linux does, to harden non-CET builds too, but that does depend on having
>> objtool so all direct branches can have their targets updated to miss
>> the UD1 instruction.
> I think it would be possible (but likely cumbersome) to arrange for
> this also without objtool.
It's only useful non-CET hardware to cross-check that things won't
explode when CET is enabled, due to mispositioned branches/etc.
An actual attacker on non-CET hardware would just adjust the function
pointer +4 to skip the UD1.
>> P.S. I'd still like to have "away %u of %u endbr64's". It occurs to me
>> that now we have check-endbr64.sh, we could `wc -l` the $VALID file and
>> re-link this back in, but then we couldn't check the final objects.
> I was thinking about this wish of yours as well; I simply didn't know how
> important you view it to have this information.
The most useful piece of information is how many ENDBR64's remain,
because those denote the extent of the attackers ability to hijack
function pointers without suffering #CP. Ideally, the number produced
at boot wants cross-checking with script which can take xen-syms,
calculate $VALID - $CF_CLOBBER and identify all the expected
runtime-active targets.
At the moment, I can guestimate based on knowing how many where disabled
at boot time, and how many were in the original build, but I don't
expect anyone else to know that an all-enabled build of Xen is ~1600
ENDBR64's.
~Andrew
On 01.03.2022 15:23, Andrew Cooper wrote:
> On 01/03/2022 13:34, Jan Beulich wrote:
>> On 01.03.2022 13:48, Andrew Cooper wrote:
>>> On 01/03/2022 11:36, Jan Beulich wrote:
>>>> Suitable compiler options are passed only when the actual feature
>>>> (XEN_IBT) is enabled, not when merely the compiler capability was found
>>>> to be available.
>>>>
>>>> Fixes: 12e3410e071e ("x86/altcall: Check and optimise altcall targets")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Hmm yes. This is fallout from separating CONFIG_HAS_CC_CET_IBT and
>>> CONFIG_XEN_IBT.
>>>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Thanks.
>>
>>>> ---
>>>> Furthermore, is "Optimised away ..." really appropriate in what
>>>> 37ed5da851b8 ("x86/altcall: Optimise away endbr64 instruction where
>>>> possible") added? If this really was an optimization (rather than
>>>> hardening), shouldn't we purge ENDBR also when !cpu_has_xen_ibt, and
>>>> then ideally all of them? Whereas if this is mainly about hardening,
>>>> wouldn't the message better say "Purged" or "Clobbered"?
>>> I did have an RFC about this. Turning ENDBR into NOP4 matters, on a
>>> CET-IBT-active system, to restrict the available options an attacker has
>>> when they have already managed to hijack a function pointer (i.e.
>>> already got a partial write gadget).
>>>
>>> From that point of view, it is hardening.
>> But then you don't say whether there's any optimization aspect here.
>> My question was really about the wording of that log message.
>
> The optimisation aspect is direct branch target +4, because that is what
> saves on decode bandwidth.
But that's the other, entirely independent ENDBR-related piece of code in
the function. All we do in the loop ahead of emitting the message here is
"add_nops(ptr, ENDBR64_LEN)". There's no counting at all of how many
times we advance a call target by 4.
Jan
© 2016 - 2024 Red Hat, Inc.