linux-kernel - Re: [PATCH v7 01/18] x86/fsgsbase/64: Fix ARCH_SET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <9040CFCD-74BD-4C17-9A01-B9B713CF6B10@intel.com>
Date:   Sun, 16 Jun 2019 15:44:11 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     Andy Lutomirski <luto@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...nel.org>,
        "H . Peter Anvin" <hpa@...or.com>, Andi Kleen <ak@...ux.intel.com>
CC:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 01/18] x86/fsgsbase/64: Fix ARCH_SET_FS/GS behaviors
 for a remote task


> On Jun 14, 2019, at 13:11, Bae, Chang Seok <chang.seok.bae@...el.com> wrote:
>> 
>> On May 8, 2019, at 10:25, Bae, Chang Seok <chang.seok.bae@...el.com> wrote:
>> 
>>> On May 8, 2019, at 03:02, Bae, Chang Seok <chang.seok.bae@...el.com> 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@...el.com>
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@...nel.org>
Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Ingo Molnar <mingo@...nel.org>
Cc: H. Peter Anvin <hpa@...or.com>
Cc: Andi Kleen <ak@...ux.intel.com>
---
 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