Re: [PATCH v7 01/18] x86/fsgsbase/64: Fix ARCH_SET_FS/GS behaviors for a remote task

[Bae, Chang Seok]

> On Jun 14, 2019, at 13:11, Bae, Chang Seok <chang.seok.bae@xxxxxxxxx> wrote:
>> 
>> On May 8, 2019, at 10:25, Bae, Chang Seok <chang.seok.bae@xxxxxxxxx> wrote:
>> 
>>> On May 8, 2019, at 03:02, Bae, Chang Seok <chang.seok.bae@xxxxxxxxx> wrote:
>>> 
>>> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>>> index 4b8ee05..3309cfe 100644
>>> --- a/arch/x86/kernel/ptrace.c
>>> +++ b/arch/x86/kernel/ptrace.c
>>> @@ -396,22 +396,12 @@ static int putreg(struct task_struct *child,
>>> 	case offsetof(struct user_regs_struct,fs_base):
>>> 		if (value >= TASK_SIZE_MAX)
>>> 			return -EIO;
>>> -		/*
>>> -		 * When changing the FS base, use do_arch_prctl_64()
>>> -		 * to set the index to zero and to set the base
>>> -		 * as requested.
>>> -		 */
>>> -		if (child->thread.fsbase != value)
>>> -			return do_arch_prctl_64(child, ARCH_SET_FS, value);
>>> +		x86_fsbase_write_cpu(child, value);
> 
> This should be x86_fsbase_write_task() instead of *cpu()
> 
>>> 		return 0;
>>> 	case offsetof(struct user_regs_struct,gs_base):
>>> -		/*
>>> -		 * Exactly the same here as the %fs handling above.
>>> -		 */
>>> 		if (value >= TASK_SIZE_MAX)
>>> 			return -EIO;
>>> -		if (child->thread.gsbase != value)
>>> -			return do_arch_prctl_64(child, ARCH_SET_GS, value);
>>> +		x86_gsbase_write_cpu(child, value);
> 
> This should be also x86_gsbase_write_task() instead of *cpu()
> 
>>> 
>> 
>> Hmm, sorry there is a glitch.  At least, intended this title to be
>> âFix ptrace-induced FS/GSBASE write behaviorâ and no changes
>> in the PRCTL. Will fix in the next version.
>> 
> 
> Hi Thomas, 
> 
> Due to the issues on this patch - my apologies, I was originally
> considering the v8. Given your re-write and comments on the last 
> patch (the documentation), at least I want/need to give my heads-up.
> 
> Thanks,
> Chang

[ Include LKML back. Unintentionally, it was missed. ]

Looks build error was reported with this. Sorry again for the noise.
Below patch was prepared to fix and to send with v8:

From c9aa7f6c7306aa46b3ecbb266989718c1b1dc85e Mon Sep 17 00:00:00 2001
From: "Chang S. Bae" <chang.seok.bae@xxxxxxxxx>
Date: Wed, 1 May 2019 08:06:45 -0700
Subject: x86/fsgsbase/64: Fix ptrace-induced FS/GSBASE writing behaviors

When a ptracer writes to a ptracee's FS/GSBASE with a different value,
the selector is also cleared. This behavior is not straightforward.

The change will make the behavior simple and sensible, as it will
(only) update the base when requested. Also, the condition check for
comparing the base is removed to make more simple. It might save a few
cycles, but this path is not performance critical.

The only recognizable downside of this change is when writing the base
if the selector is already nonzero. The base will be reloaded according
to the selector. But the case is highly unexpected in real usages.

Suggested-by: Andy Lutomirski <luto@xxxxxxxxxx>
Signed-off-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
Cc: Ingo Molnar <mingo@xxxxxxxxxx>
Cc: H. Peter Anvin <hpa@xxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
---
 arch/x86/kernel/ptrace.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index a166c96..3108cdc 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -397,22 +397,12 @@ static int putreg(struct task_struct *child,
 	case offsetof(struct user_regs_struct,fs_base):
 		if (value >= TASK_SIZE_MAX)
 			return -EIO;
-		/*
-		 * When changing the FS base, use do_arch_prctl_64()
-		 * to set the index to zero and to set the base
-		 * as requested.
-		 */
-		if (child->thread.fsbase != value)
-			return do_arch_prctl_64(child, ARCH_SET_FS, value);
+		x86_fsbase_write_task(child, value);
 		return 0;
 	case offsetof(struct user_regs_struct,gs_base):
-		/*
-		 * Exactly the same here as the %fs handling above.
-		 */
 		if (value >= TASK_SIZE_MAX)
 			return -EIO;
-		if (child->thread.gsbase != value)
-			return do_arch_prctl_64(child, ARCH_SET_GS, value);
+		x86_gsbase_write_task(child, value);
 		return 0;
 #endif
 	}
-- 
2.7.4