| Message ID | 0e3594ff-c637-46f2-bc95-7a26b5471b86@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/altcall: silence undue warning | expand |
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
--- 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;
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"?