diff mbox series

[2/3] x86/entry: Make #PF/NMI/INT0x82 more amenable to livepatching

Message ID 20240122181714.1543738-3-andrew.cooper3@citrix.com (mailing list archive)
State New
Headers show
Series x86/entry: ELF fixes and improvments | expand

Commit Message

Andrew Cooper Jan. 22, 2024, 6:17 p.m. UTC 
It is bad form to have inter-function fallthrough.  It only functions right
now because alignment padding bytes are NOPs.

However, it also interferes with livepatching binary diffs, because the
implicit grouping of the two functions isn't expressed in the ELF metadata.

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: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/x86_64/compat/entry.S | 1 +
 xen/arch/x86/x86_64/entry.S        | 3 +++
 2 files changed, 4 insertions(+)

Comments

Jan Beulich Jan. 23, 2024, 9:22 a.m. UTC | #1 
On 22.01.2024 19:17, Andrew Cooper wrote:
> It is bad form to have inter-function fallthrough.  It only functions right
> now because alignment padding bytes are NOPs.

But that's a requirement anyway in executable sections.

> --- a/xen/arch/x86/x86_64/compat/entry.S
> +++ b/xen/arch/x86/x86_64/compat/entry.S
> @@ -29,6 +29,7 @@ FUNC(entry_int82)
>  
>          mov   %rsp, %rdi
>          call  do_entry_int82
> +        jmp   compat_test_all_events
>  END(entry_int82)
>  
>  /* %rbx: struct vcpu */
> diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> index c3f6b667a72a..fc64ef1fd460 100644
> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -723,7 +723,9 @@ END(common_interrupt)
>  FUNC(entry_PF)
>          ENDBR64
>          movl  $X86_EXC_PF, 4(%rsp)
> +        jmp   handle_exception
>  END(entry_PF)
> +
>  /* No special register assumptions. */
>  FUNC(handle_exception, 0)
>          ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
> @@ -1023,6 +1025,7 @@ FUNC(entry_NMI)
>          ENDBR64
>          pushq $0
>          movl  $X86_EXC_NMI, 4(%rsp)
> +        jmp   handle_ist_exception
>  END(entry_NMI)
>  
>  FUNC(handle_ist_exception)

Hmm, so here you (partly) do what I was meaning to do in the one patch
left from the entry point annotations series, "common: honor
CONFIG_CC_SPLIT_SECTIONS also for assembly functions". However, I'm
wrapping the JMPs there in #ifdef CONFIG_CC_SPLIT_SECTIONS. Thoughts?
I view the JMPs as pretty useless otherwise, even if there is a
small risk of a future code change not respecting the ordering
requirements. Yet such would be noticed pretty quickly, I suppose.

Jan
Roger Pau Monné Jan. 23, 2024, 1:37 p.m. UTC | #2 
On Tue, Jan 23, 2024 at 10:22:10AM +0100, Jan Beulich wrote:
> On 22.01.2024 19:17, Andrew Cooper wrote:
> > It is bad form to have inter-function fallthrough.  It only functions right
> > now because alignment padding bytes are NOPs.
> 
> But that's a requirement anyway in executable sections.

Really?  I was under the impression we wanted to replace the padding
nops with rets maybe, or even poison the padding with int3 or ud2.

> > --- a/xen/arch/x86/x86_64/compat/entry.S
> > +++ b/xen/arch/x86/x86_64/compat/entry.S
> > @@ -29,6 +29,7 @@ FUNC(entry_int82)
> >  
> >          mov   %rsp, %rdi
> >          call  do_entry_int82
> > +        jmp   compat_test_all_events
> >  END(entry_int82)
> >  
> >  /* %rbx: struct vcpu */
> > diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
> > index c3f6b667a72a..fc64ef1fd460 100644
> > --- a/xen/arch/x86/x86_64/entry.S
> > +++ b/xen/arch/x86/x86_64/entry.S
> > @@ -723,7 +723,9 @@ END(common_interrupt)
> >  FUNC(entry_PF)
> >          ENDBR64
> >          movl  $X86_EXC_PF, 4(%rsp)
> > +        jmp   handle_exception
> >  END(entry_PF)
> > +
> >  /* No special register assumptions. */
> >  FUNC(handle_exception, 0)
> >          ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
> > @@ -1023,6 +1025,7 @@ FUNC(entry_NMI)
> >          ENDBR64
> >          pushq $0
> >          movl  $X86_EXC_NMI, 4(%rsp)
> > +        jmp   handle_ist_exception
> >  END(entry_NMI)
> >  
> >  FUNC(handle_ist_exception)
> 
> Hmm, so here you (partly) do what I was meaning to do in the one patch
> left from the entry point annotations series, "common: honor
> CONFIG_CC_SPLIT_SECTIONS also for assembly functions". However, I'm
> wrapping the JMPs there in #ifdef CONFIG_CC_SPLIT_SECTIONS. Thoughts?
> I view the JMPs as pretty useless otherwise, even if there is a
> small risk of a future code change not respecting the ordering
> requirements. Yet such would be noticed pretty quickly, I suppose.

I think it's clearer with the jumps.

Thanks, Roger.
Jan Beulich Jan. 23, 2024, 1:43 p.m. UTC | #3 
On 23.01.2024 14:37, Roger Pau Monné wrote:
> On Tue, Jan 23, 2024 at 10:22:10AM +0100, Jan Beulich wrote:
>> On 22.01.2024 19:17, Andrew Cooper wrote:
>>> It is bad form to have inter-function fallthrough.  It only functions right
>>> now because alignment padding bytes are NOPs.
>>
>> But that's a requirement anyway in executable sections.
> 
> Really?  I was under the impression we wanted to replace the padding
> nops with rets maybe, or even poison the padding with int3 or ud2.

Well, that would be a decision of ours. Which then imo can't be described as
"only functions right now because ..." The assembler can't[1] use other than
NOPs by default, as it can't know whether fall-through is intended.

Jan

[1] minus bugs - see e.g. https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=d164359dbc14c8ae4c7a117d236f5b7de4af671a
Roger Pau Monné Jan. 24, 2024, 9:22 a.m. UTC | #4 
On Tue, Jan 23, 2024 at 02:43:15PM +0100, Jan Beulich wrote:
> On 23.01.2024 14:37, Roger Pau Monné wrote:
> > On Tue, Jan 23, 2024 at 10:22:10AM +0100, Jan Beulich wrote:
> >> On 22.01.2024 19:17, Andrew Cooper wrote:
> >>> It is bad form to have inter-function fallthrough.  It only functions right
> >>> now because alignment padding bytes are NOPs.
> >>
> >> But that's a requirement anyway in executable sections.
> > 
> > Really?  I was under the impression we wanted to replace the padding
> > nops with rets maybe, or even poison the padding with int3 or ud2.
> 
> Well, that would be a decision of ours. Which then imo can't be described as
> "only functions right now because ..." The assembler can't[1] use other than
> NOPs by default, as it can't know whether fall-through is intended.

So it's not a strict requirement of ELF that padding is done using
nops, it's just the default decision of the assembler because it
doesn't know better.

Thanks, Roger.
diff mbox series

Patch

diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 49811a56e965..4fbd89cea1a9 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -29,6 +29,7 @@  FUNC(entry_int82)
 
         mov   %rsp, %rdi
         call  do_entry_int82
+        jmp   compat_test_all_events
 END(entry_int82)
 
 /* %rbx: struct vcpu */
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index c3f6b667a72a..fc64ef1fd460 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -723,7 +723,9 @@  END(common_interrupt)
 FUNC(entry_PF)
         ENDBR64
         movl  $X86_EXC_PF, 4(%rsp)
+        jmp   handle_exception
 END(entry_PF)
+
 /* No special register assumptions. */
 FUNC(handle_exception, 0)
         ALTERNATIVE "", clac, X86_FEATURE_XEN_SMAP
@@ -1023,6 +1025,7 @@  FUNC(entry_NMI)
         ENDBR64
         pushq $0
         movl  $X86_EXC_NMI, 4(%rsp)
+        jmp   handle_ist_exception
 END(entry_NMI)
 
 FUNC(handle_ist_exception)