xen/arch/x86/traps.c | 83 ++++++++++++++++++++++++++-------------------------- 1 file changed, 42 insertions(+), 41 deletions(-)
Some versions of GCC complain with:
traps.c:405:22: error: 'get_shstk_bottom' defined but not used [-Werror=unused-function]
static unsigned long get_shstk_bottom(unsigned long sp)
^~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
Change #ifdef to if ( IS_ENABLED(...) ) to make the sole user of
get_shstk_bottom() visible to the compiler.
Fixes: 35727551c070 ("x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Not actually tested. I don't seem to have a new enough GCC to hand.
Most of the delta here is indentation. This diff is more easily reviewed with
`git show --ignore-all-space`
---
xen/arch/x86/traps.c | 83 ++++++++++++++++++++++++++--------------------------
1 file changed, 42 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 30eefbad4863..4a0e498b4c21 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -780,55 +780,56 @@ static void do_reserved_trap(struct cpu_user_regs *regs)
static void fixup_exception_return(struct cpu_user_regs *regs,
unsigned long fixup)
{
-#ifdef CONFIG_XEN_SHSTK
- unsigned long ssp, *ptr, *base;
+ if ( IS_ENABLED(CONFIG_XEN_SHSTK) )
+ {
+ unsigned long ssp, *ptr, *base;
- asm ( "rdsspq %0" : "=r" (ssp) : "0" (1) );
- if ( ssp == 1 )
- goto shstk_done;
+ asm ( "rdsspq %0" : "=r" (ssp) : "0" (1) );
+ if ( ssp == 1 )
+ goto shstk_done;
- ptr = _p(ssp);
- base = _p(get_shstk_bottom(ssp));
+ ptr = _p(ssp);
+ base = _p(get_shstk_bottom(ssp));
- for ( ; ptr < base; ++ptr )
- {
- /*
- * Search for %rip. The shstk currently looks like this:
- *
- * ... [Likely pointed to by SSP]
- * %cs [== regs->cs]
- * %rip [== regs->rip]
- * SSP [Likely points to 3 slots higher, above %cs]
- * ... [call tree to this function, likely 2/3 slots]
- *
- * and we want to overwrite %rip with fixup. There are two
- * complications:
- * 1) We cant depend on SSP values, because they won't differ by 3
- * slots if the exception is taken on an IST stack.
- * 2) There are synthetic (unrealistic but not impossible) scenarios
- * where %rip can end up in the call tree to this function, so we
- * can't check against regs->rip alone.
- *
- * Check for both regs->rip and regs->cs matching.
- */
- if ( ptr[0] == regs->rip && ptr[1] == regs->cs )
+ for ( ; ptr < base; ++ptr )
{
- asm ( "wrssq %[fix], %[stk]"
- : [stk] "=m" (ptr[0])
- : [fix] "r" (fixup) );
- goto shstk_done;
+ /*
+ * Search for %rip. The shstk currently looks like this:
+ *
+ * ... [Likely pointed to by SSP]
+ * %cs [== regs->cs]
+ * %rip [== regs->rip]
+ * SSP [Likely points to 3 slots higher, above %cs]
+ * ... [call tree to this function, likely 2/3 slots]
+ *
+ * and we want to overwrite %rip with fixup. There are two
+ * complications:
+ * 1) We cant depend on SSP values, because they won't differ by
+ * 3 slots if the exception is taken on an IST stack.
+ * 2) There are synthetic (unrealistic but not impossible)
+ * scenarios where %rip can end up in the call tree to this
+ * function, so we can't check against regs->rip alone.
+ *
+ * Check for both regs->rip and regs->cs matching.
+ */
+ if ( ptr[0] == regs->rip && ptr[1] == regs->cs )
+ {
+ asm ( "wrssq %[fix], %[stk]"
+ : [stk] "=m" (ptr[0])
+ : [fix] "r" (fixup) );
+ goto shstk_done;
+ }
}
- }
- /*
- * We failed to locate and fix up the shadow IRET frame. This could be
- * due to shadow stack corruption, or bad logic above. We cannot continue
- * executing the interrupted context.
- */
- BUG();
+ /*
+ * We failed to locate and fix up the shadow IRET frame. This could
+ * be due to shadow stack corruption, or bad logic above. We cannot
+ * continue executing the interrupted context.
+ */
+ BUG();
+ }
shstk_done:
-#endif /* CONFIG_XEN_SHSTK */
/* Fixup the regular stack. */
regs->rip = fixup;
--
2.11.0
On 17.08.2021 12:56, Andrew Cooper wrote:
> Some versions of GCC complain with:
>
> traps.c:405:22: error: 'get_shstk_bottom' defined but not used [-Werror=unused-function]
> static unsigned long get_shstk_bottom(unsigned long sp)
> ^~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Change #ifdef to if ( IS_ENABLED(...) ) to make the sole user of
> get_shstk_bottom() visible to the compiler.
>
> Fixes: 35727551c070 ("x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Not actually tested. I don't seem to have a new enough GCC to hand.
Compile-tested-by: Jan Beulich <jbeulich@suse.com>
Jan
On Tue, Aug 17, 2021 at 11:56:56AM +0100, Andrew Cooper wrote:
> Some versions of GCC complain with:
>
> traps.c:405:22: error: 'get_shstk_bottom' defined but not used [-Werror=unused-function]
> static unsigned long get_shstk_bottom(unsigned long sp)
> ^~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
>
> Change #ifdef to if ( IS_ENABLED(...) ) to make the sole user of
> get_shstk_bottom() visible to the compiler.
>
> Fixes: 35727551c070 ("x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
> Not actually tested. I don't seem to have a new enough GCC to hand.
I have just compile-tested it and it seems to fix the issue (indeed it
failed before with CONFIG_XEN_SHSTK disabled).
> Most of the delta here is indentation. This diff is more easily reviewed with
> `git show --ignore-all-space`
Wouldn't this make the compiler include the code even if
CONFIG_XEN_SHSTK is disabled (not a huge issue...)? Or is it smart
enough to optimize it out in that case?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
On 17/08/2021 12:14, Marek Marczykowski-Górecki wrote:
> On Tue, Aug 17, 2021 at 11:56:56AM +0100, Andrew Cooper wrote:
>> Some versions of GCC complain with:
>>
>> traps.c:405:22: error: 'get_shstk_bottom' defined but not used [-Werror=unused-function]
>> static unsigned long get_shstk_bottom(unsigned long sp)
>> ^~~~~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>>
>> Change #ifdef to if ( IS_ENABLED(...) ) to make the sole user of
>> get_shstk_bottom() visible to the compiler.
>>
>> Fixes: 35727551c070 ("x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>>
>> Not actually tested. I don't seem to have a new enough GCC to hand.
> I have just compile-tested it and it seems to fix the issue (indeed it
> failed before with CONFIG_XEN_SHSTK disabled).
Oh, thanks!
>
>> Most of the delta here is indentation. This diff is more easily reviewed with
>> `git show --ignore-all-space`
> Wouldn't this make the compiler include the code even if
> CONFIG_XEN_SHSTK is disabled (not a huge issue...)? Or is it smart
> enough to optimize it out in that case?
Its a trivial dead-code elimination example, and yes - the compiler is
smart enough.
~Andrew
On Tue, Aug 17, 2021 at 12:17:31PM +0100, Andrew Cooper wrote:
> On 17/08/2021 12:14, Marek Marczykowski-Górecki wrote:
> > On Tue, Aug 17, 2021 at 11:56:56AM +0100, Andrew Cooper wrote:
> >> Some versions of GCC complain with:
> >>
> >> traps.c:405:22: error: 'get_shstk_bottom' defined but not used [-Werror=unused-function]
> >> static unsigned long get_shstk_bottom(unsigned long sp)
> >> ^~~~~~~~~~~~~~~~
> >> cc1: all warnings being treated as errors
> >>
> >> Change #ifdef to if ( IS_ENABLED(...) ) to make the sole user of
> >> get_shstk_bottom() visible to the compiler.
> >>
> >> Fixes: 35727551c070 ("x86/cet: Fix shskt manipulation error with BUGFRAME_{warn,run_fn}")
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Wei Liu <wl@xen.org>
> >> CC: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> >>
> >> Not actually tested. I don't seem to have a new enough GCC to hand.
> > I have just compile-tested it and it seems to fix the issue (indeed it
> > failed before with CONFIG_XEN_SHSTK disabled).
>
> Oh, thanks!
And I can confirm it doesn't break anything runtime (but that's pretty
obvious looking at the patch).
> >> Most of the delta here is indentation. This diff is more easily reviewed with
> >> `git show --ignore-all-space`
> > Wouldn't this make the compiler include the code even if
> > CONFIG_XEN_SHSTK is disabled (not a huge issue...)? Or is it smart
> > enough to optimize it out in that case?
>
> Its a trivial dead-code elimination example, and yes - the compiler is
> smart enough.
:)
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
© 2016 - 2024 Red Hat, Inc.