Re: [PATCH v3 5/6] x86/alternative: Use a single access in text_poke() where possible

[Alexandre Chartre]

On 01/10/2019 07:04 PM, Sean Christopherson wrote:
> On Thu, Jan 10, 2019 at 12:57:57PM -0500, Steven Rostedt wrote:
> > On Thu, 10 Jan 2019 09:42:57 -0800
> > Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
> >
> > > On Thu, Jan 10, 2019 at 12:32:43PM -0500, Steven Rostedt wrote:
> > > > On Thu, 10 Jan 2019 11:20:04 -0600
> > > > Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> > > >
> > > >
> > > > > > While I can't find a reason for hypervisors to emulate this instruction,
> > > > > > smarter people might find ways to turn it into a security exploit.
> > > > >
> > > > > Interesting point... but I wonder if it's a realistic concern.  BTW,
> > > > > text_poke_bp() also relies on undocumented behavior.
> > > >
> > > > But we did get an official OK from Intel that it will work. Took a bit
> > > > of arm twisting to get them to do so, but they did. And it really is
> > > > pretty robust.
> > >
> > > Did we (they?) list any caveats for this behavior?  E.g. I'm fairly
> > > certain atomicity guarantees go out the window if WC memtype is used.
> >
> > Note, the text_poke_bp() process was this: (nothing to do with atomic
> > guarantees)
> >
> > add breakpoint (one byte) to instruction.
> >
> > Sync all cores (send an IPI to each one).
> >
> > change the back half of the instruction (the rest of the instruction
> > after the breakpoint).
> >
> > Sync all cores
> >
> > Remove the breakpoint with the new byte of the new instruction.
> >
> >
> > What atomicity guarantee does the above require?
>
> I was asking in the context of static calls.  My understanding is that
> the write to change the imm32 of the CALL needs to be atomic from a
> code fetch perspective so that we don't jump to a junk address.

To avoid any issue with live patching the call instruction, what about
toggling between two call instructions: one would be the currently active
call, while the other would currently be inactive but to be used after a
change is made. You can safely patch the inactive call and then toggle
the call flow (using a jump label) between the active and inactive calls.

So instead of having a single call instruction:

	call function

You would have:

	STATIC_JUMP_IF_TRUE label, key
	call function1
	jmp done
label:
	call function2
done:

If the key is set so that function1 is currently called then you can
safely update the call instruction for function2. Once this is done,
just flip the key to make the function2 call active. On a next update,
you would, of course, have to switch and update the call for function1.


alex.