On 11/07/2018 10:33 AM, Waiman Long wrote:
> On 11/06/2018 07:18 PM, Tim Chen wrote:
>> Thomas,
>>
>>>>> 2) Add _TIF_UPDATE_SPEC_CTRL to the SYSCALL_EXIT_WORK_FLAGS and handle it
>>>>> in the slow work path.
>>>> There can be tasks that don't do any syscalls, and it seems like we can
>>>> have MSRs getting out of sync?
>>> Setting the TIF flag directly in a remote task is wrong. It needs to be
>>> handled when the _TIF_UPDATE_SPEC_CTRL is evaluated, i.e. the information
>>> needs to be stored process wide e.g. in task->mm.
>>>
>>> But yes, if the remote task runs in user space forever, it won't
>>> help. Though the point is that dumpable is usually set when the process
>>> starts, so it's probably mostly a theoretical issue.
>>>
>> I took a crack to implement what you suggested to update
>> remote task's flag and remote SPEC_CTRL MSR on the syscall exit slow path.
>>
>> This looks reasobale?
>>
>> Tim
>>
>>
>> ------------
>>
>> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
>> index 3b2490b..614594a 100644
>> --- a/arch/x86/entry/common.c
>> +++ b/arch/x86/entry/common.c
>> @@ -216,7 +216,7 @@ __visible inline void prepare_exit_to_usermode(struct
>> pt_regs *regs)
>>
>> #define SYSCALL_EXIT_WORK_FLAGS \
>> (_TIF_SYSCALL_TRACE | _TIF_SYSCALL_AUDIT | \
>> - _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT)
>> + _TIF_SINGLESTEP | _TIF_SYSCALL_TRACEPOINT | _TIF_UPDATE_SPEC_CTRL)
>>
>> static void syscall_slow_exit_work(struct pt_regs *regs, u32 cached_flags)
>> {
>> @@ -227,6 +227,8 @@ static void syscall_slow_exit_work(struct pt_regs *regs,
>> u32 cached_flags)
>> if (cached_flags & _TIF_SYSCALL_TRACEPOINT)
>> trace_sys_exit(regs, regs->ax);
>>
>> + if (cached_flags & _TIF_UPDATE_SPEC_CTRL)
>> + spec_ctrl_do_pending_update();
>> /*
>> * If TIF_SYSCALL_EMU is set, we only get here because of
>> * TIF_SINGLESTEP (i.e. this is PTRACE_SYSEMU_SINGLESTEP).
>> diff --git a/arch/x86/include/asm/nospec-branch.h
>> b/arch/x86/include/asm/nospec-branch.h
>> index c59a6c4..f124597 100644
>> --- a/arch/x86/include/asm/nospec-branch.h
>> +++ b/arch/x86/include/asm/nospec-branch.h
>> @@ -276,6 +276,8 @@ static inline void
>> indirect_branch_prediction_barrier(void)
>> alternative_msr_write(MSR_IA32_PRED_CMD, val, X86_FEATURE_USE_IBPB);
>> }
>>
>> +void spec_ctrl_do_pending_update(void);
>> +
>> /* The Intel SPEC CTRL MSR base value cache */
>> extern u64 x86_spec_ctrl_base;
>>
>> diff --git a/arch/x86/include/asm/thread_info.h
>> b/arch/x86/include/asm/thread_info.h
>> index 4f6a7a9..b78db59 100644
>> --- a/arch/x86/include/asm/thread_info.h
>> +++ b/arch/x86/include/asm/thread_info.h
>> @@ -97,6 +97,7 @@ struct thread_info {
>> #define TIF_USER_RETURN_NOTIFY 14 /* Notify kernel of userspace
>> return */
>> #define TIF_PATCH_PENDING 15 /* Pending live patching update */
>> #define TIF_FSCHECK 16 /* Check FS is USER_DS on return */
>> +#define TIF_UPDATE_SPEC_CTRL 17 /* Pending update of
>> speculation control */
>>
>> /* Task status */
>> #define TIF_UPROBE 18 /* Breakpointed or singlestepping */
>> @@ -131,6 +132,7 @@ struct thread_info {
>> #define _TIF_USER_RETURN_NOTIFY (1 << TIF_USER_RETURN_NOTIFY)
>> #define _TIF_PATCH_PENDING (1 << TIF_PATCH_PENDING)
>> #define _TIF_FSCHECK (1 << TIF_FSCHECK)
>> +#define _TIF_UPDATE_SPEC_CTRL (1 << TIF_UPDATE_SPEC_CTRL)
>>
>> #define _TIF_UPROBE (1 << TIF_UPROBE)
>> #define _TIF_MEMDIE (1 << TIF_MEMDIE)
>> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>> index 4c15c86..d82d3f8 100644
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -14,6 +14,8 @@
>> #include <linux/module.h>
>> #include <linux/nospec.h>
>> #include <linux/prctl.h>
>> +#include <linux/sched/coredump.h>
>> +#include <linux/sched/signal.h>
>>
>> #include <asm/spec-ctrl.h>
>> #include <asm/cmdline.h>
>> @@ -770,6 +772,69 @@ static int ssb_prctl_set(struct task_struct *task,
>> unsigned long ctrl)
>> return 0;
>> }
>>
>> +static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
>> +{
>> + bool update = false;
>> +
>> + if (stibp_on)
>> + update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
>> + else
>> + update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
>> +
>> + if (tsk == current && update)
>> + speculation_ctrl_update_current();
>> +}
>> +
>> +void spec_ctrl_do_pending_update(void)
>> +{
>> + if (!static_branch_unlikely(&spectre_v2_app_lite))
>> + return;
>> +
>> + if (!current->mm)
>> + return;
>> +
>> + if (get_dumpable(current->mm) != SUID_DUMP_USER)
>> + set_tsk_thread_flag(current, TIF_STIBP);
>> + else
>> + clear_tsk_thread_flag(current, TIF_STIBP);
>> +
>> + clear_tsk_thread_flag(current, TIF_UPDATE_SPEC_CTRL);
>> + speculation_ctrl_update_current();
>> +}
>> +
>> +int arch_update_spec_ctrl_restriction(struct task_struct *task)
>> +{
>> + unsigned long flags;
>> + struct task_struct *t;
>> + bool stibp_on = false;
>> +
>> + if (!static_branch_unlikely(&spectre_v2_app_lite))
>> + return 0;
>> +
>> + if (!task->mm)
>> + return -EINVAL;
>> +
>> + if (!lock_task_sighand(task, &flags))
>> + return -ESRCH;
>> +
>> + if (get_dumpable(task->mm) != SUID_DUMP_USER)
>> + stibp_on = true;
>> +
>> + for_each_thread(task, t) {
>> + if (task_cpu(task) == smp_processor_id())
>> + set_task_stibp(task, stibp_on);
>
> I think "t" is the iterator, not "task". BTW, a thread is on the same
> CPU doesn't mean it is running. Should you just check "(t == current)" here?
Ah yes, should be t. t==current is checked in set_task_stibp.
Tim
>
>> + else if (test_tsk_thread_flag(task, TIF_STIBP) != stibp_on)
>> + set_tsk_thread_flag(task, TIF_UPDATE_SPEC_CTRL);
>> + }
>> +
>> + unlock_task_sighand(task, &flags);
>> + return 0;
>> +}
>> +
>> int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>> unsigned long ctrl)
>> {
>
> Cheers,
> Longman
>
