On Tue, Mar 14, 2017 at 9:44 AM, H. Peter Anvin [off-list ref] wrote:
On 03/14/17 09:29, Thomas Garnier wrote:
quoted
We want to enforce the address limit by default, not only when
CONFIG_BUG_ON_DATA_CORRUPTION is enabled. I tested this one:
/* Check user-mode state on fast path return. */
movq PER_CPU_VAR(current_task), %rax
btq $63, TASK_addr_limit(%rax)
jnc 1f
#ifdef CONFIG_BUG_ON_DATA_CORRUPTION
call syscall_return_slowpath
jmp return_from_SYSCALL_64
#else
movq $TASK_SIZE_MAX, %rcx
movq %rcx, TASK_addr_limit(%rax)
#endif
1:
I saw that syscall_return_slowpath is supposed to be called not jumped
to. I could just call verify_pre_usermode_state that would be about
the same.
I wanted to comment on that thing: why on earth isn't
verify_pre_usermode_state() an inline? Making it an out-of-line
function adds pointless extra overhead to the C code when we are talking
about a few instructions.
Because outside of arch specific implementation it is called by each
syscall handler. it will increase the code size a lot.
Second, you never do a branch-around to handle an exceptional condition
on the fast path: you jump *out of line* to handle the special
condition; a forward branch is preferred since it is slightly more
likely to be predicted not taken.
Now, I finally had a chance to actually look at the full file (I was
preoccupied yesterday), and am a bit disappointed, to say the least.
First of all, the jump target you need is only a handful of instructions
further down the code path; you need to do *exactly* that is done when
the test of _TIF_ALLWORK_MASK right above is tested! Not only that, but
you already have PER_CPU_VAR(current_task) in %r11 just ready to be
used! This was all in the three instructions immediately prior to the
code you modified...
Correct.
So, all you'd need would be:
movq $TASK_SIZE_MAX, %rcx
cmpq %rcx, TASK_addr_limit(%r11)
jne 1f
We even get a short jump instruction!
(Using bt saves one more instruction, but see previous caveats about it.)
Okay that seems fair to me. Andy what do you think? (Given you
suggested the bt). Ingo?
Thanks for the feedback.
-hpa
--
Thomas