lkml.org 
[lkml]   [2014]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC PATCH] x86, entry: Switch stacks on a paranoid entry from userspace
Andy,

As I said many times I do not understand asm ;) so most probably I missed
something but let me ask anyway.

On 11/11, Andy Lutomirski wrote:
>
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -1064,6 +1064,9 @@ ENTRY(\sym)
> CFI_ADJUST_CFA_OFFSET ORIG_RAX-R15
>
> .if \paranoid
> + CFI_REMEMBER_STATE
> + testl $3, CS(%rsp) /* If coming from userspace, switch */
> + jnz 1f /* stacks. */
> call save_paranoid
> .else
> call error_entry
> @@ -1104,6 +1107,36 @@ ENTRY(\sym)
> jmp error_exit /* %ebx: no swapgs flag */
> .endif
>
> + .if \paranoid
> + CFI_RESTORE_STATE
> + /*
> + * Paranoid entry from userspace. Switch stacks and treat it
> + * as a normal entry. This means that paranoid handlers
> + * run in real process context if user_mode(regs).
> + */
> +1:
> + call error_entry
> +
> + DEFAULT_FRAME 0
> +
> + movq %rsp,%rdi /* pt_regs pointer */
> + call sync_regs

Can't we simplify sync_regs() then?

> @@ -1324,8 +1357,6 @@ ENTRY(paranoid_exit)
> TRACE_IRQS_OFF_DEBUG
> testl %ebx,%ebx /* swapgs needed? */
> jnz paranoid_restore
> - testl $3,CS(%rsp)
> - jnz paranoid_userspace
> paranoid_swapgs:

Looks like this label can die.

> -paranoid_userspace:
> - GET_THREAD_INFO(%rcx)
> - movl TI_flags(%rcx),%ebx
> - andl $_TIF_WORK_MASK,%ebx
> - jz paranoid_swapgs
> - movq %rsp,%rdi /* &pt_regs */
> - call sync_regs
> - movq %rax,%rsp /* switch stack for scheduling */
> - testl $_TIF_NEED_RESCHED,%ebx
> - jnz paranoid_schedule
> - movl %ebx,%edx /* arg3: thread flags */
> - TRACE_IRQS_ON
> - ENABLE_INTERRUPTS(CLBR_NONE)
> - xorl %esi,%esi /* arg2: oldset */
> - movq %rsp,%rdi /* arg1: &pt_regs */
> - call do_notify_resume

So, before this patch we use _TIF_WORK_MASK to decide if we need to call
do_notify_resume().

After this patch we jump to error_exit and it checks the same _TIF_WORK_MASK.
But note that retint_careful->retint_careful checks another mask,
_TIF_DO_NOTIFY_MASK.

So it seems to me we can miss (say) TIF_UPROBE after int3 handler, no?

Yes, even _if_ I am right we should blame these masks, _TIF_DO_NOTIFY_MASK
should probably include _TIF_UPROBE (and afaics in this case we can remove
set_thread_flag(TIF_NOTIFY_RESUME) in uprobe_deny_signal()).

And in any case, can't we cleanup _TIF_WORK_MASK and _TIF_ALLWORK_MASK?
IMHO, they should clearly define which bits we want to check.

Oleg.



\
 
 \ /
  Last update: 2014-11-12 23:41    [W:0.700 / U:0.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site