Re: [PATCH v15 13/25] x86/reboot: Add ljmp instructions to stacktool whitelist

[Ingo Molnar]

* Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:

> Some of the 'weird' cases:
> 
> - Some functions use 'ljmp' or 'lret'.  After looking at this some more,
>   I think it would be safe for stacktool to translate the use of these
>   instructions to mean "I know what I'm doing" and just ignore the
>   function.  100% of the functions are safe to ignore anyway.  So we can
>   get rid of those markers and just make stacktool smarter.

Yeah.

So the worry people have is that once such annotations get in, the incentive to 
solve them at the tooling level decreases and stays on a TODO list for a long time 
;-)

> - xen_cpuid() uses some custom xen instructions which start with
>   XEN_EMULATE_PREFIX.  It corresponds to the following x86 instructions:
> 
>     ffffffff8107e572:       0f 0b                   ud2
>     ffffffff8107e574:       78 65                   js ffffffff8107e5db <xen_get_debugreg+0xa>
>     ffffffff8107e576:       6e                      outsb %ds:(%rsi),(%dx)
> 
>   Apparently(?) xen treats the ud2 special when it's followed by "78 65
>   6e".  This is confusing for stacktool because ud2 is normally a dead
>   end, and it thinks the instructions after it will never run.
>   
>   (In theory stacktool could be taught to understand this hack, but
>   that's a bad idea IMO)

Yeah, probably. Annotating Xen special code looks acceptable.

> - The error path in arch/x86/net/bpf_jit.S uses 'leaveq' to do a double
>   return so that it returns from its caller's context.  stacktool
>   doesn't know how to distinguish this from a frame pointer programming
>   bug.  I think the only way to avoid a whitelist marker here would be
>   to rewrite the bpf code to conform with more traditional rbp usage
>   (but I don't know if that would really be a good idea because it would
>   probably result in slower/more code).

I think annotation is fine in this case.

> - __bpf_prog_run() uses a jump table:
> 
>     goto *jumptable[insn->code];
> 
>   stacktool doesn't have an x86 emulator, so it doesn't know how to
>   deterministically follow all possible branches for a dynamic jump.
> 
> - schedule() mucks with the frame pointer which is normally not allowed.

That is obviously a special case too.

As long as no 'usual' C function gets annotated as doing something weird with the 
stack frame, I'm fine with this approach.

Thanks,

	Ingo