linux-kernel - Re: [PATCH] x86: ptrace debugreg checks rewrite

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20090623094728.GA24612@x200.localdomain>
Date:	Tue, 23 Jun 2009 13:47:28 +0400
From:	Alexey Dobriyan <adobriyan@...il.com>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	Frédéric Weisbecker <fweisbec@...il.com>,
	mingo@...hat.com, tglx@...utronix.de, hpa@...or.com,
	x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: ptrace debugreg checks rewrite

On Tue, Jun 23, 2009 at 10:55:50AM +0200, Ingo Molnar wrote:
> 
> * Alexey Dobriyan <adobriyan@...il.com> wrote:
> 
> > This is a mess.
> > 
> > Pre unified-x86 code did check for breakpoint addr
> > to be "< TASK_SIZE - 3 (or 7)". This was fine from security POV,
> > but banned valid breakpoint usage when address is close to TASK_SIZE.
> > E. g. 1-byte breakpoint at TASK_SIZE - 1 should be allowed, but it wasn't.
> > 
> > Then came commit 84929801e14d968caeb84795bfbb88f04283fbd9
> > ("[PATCH] x86_64: TASK_SIZE fixes for compatibility mode processes")
> > which for some reason touched ptrace as well and made effective
> > TASK_SIZE of 32-bit process depending on IA32_PAGE_OFFSET
> > which is not a constant!:
> > 
> > 	#define IA32_PAGE_OFFSET ((current->personality & ADDR_LIMIT_3GB) ? 0xc0000000 : 0xFFFFe000)
> > 				   ^^^^^^^
> > Maximum addr for breakpoint became dependent on personality of ptracer.
> > 
> > Commit also relaxed danger zone for 32-bit processes from 8 bytes to 4
> > not taking into account that 8-byte wide breakpoints are possible even
> > for 32-bit processes. This was fine, however, because 64-bit kernel
> > addresses are too far from 32-bit ones.
> > 
> > Then came utrace with commit 2047b08be67b70875d8765fc81d34ce28041bec3
> > ("x86: x86 ptrace getreg/putreg merge") which copy-pasted and ifdeffed 32-bit
> > part of TASK_SIZE_OF() leaving 8-byte issue as-is.
> > 
> > So, what patch fixes?
> > 1) Too strict logic near TASK_SIZE boundary -- as long as we don't cross
> >    TASK_SIZE_MAX, we're fine.
> > 2) Too smart logic of using breakpoints over non-existent kernel
> >    boundary -- we should only protect against setting up after
> >    TASK_SIZE_MAX, the rest is none of kernel business. This fixes
> >    IA32_PAGE_OFFSET beartrap as well.
> > 
> > As a bonus, remove uberhack and big comment determining DR7 validness,
> > rewrite with clear algorithm when it's obvious what's going on.
> > 
> > Make DR validness checker suitable for C/R. On restart DR registers
> > must be checked the same way they are checked on PTRACE_POKEUSR.
> > 
> > Question 1: TIF_DEBUG can set even if none of breakpoints is turned on,
> > should this be optimized?
> > 
> > Question 2: Breakpoints are allowed to be globally enabled, is this a
> > security risk?
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@...il.com>
> 
> Please base this on the latest x86 tree:
> 
>   http://people.redhat.com/mingo/tip.git/README
> 
> which has the hw-debug rework with debug register ops abstracted out 
> already - making your patch not apply at all.

Why haven't you applied this patch 1.5 months ago when it was ready?
Patch hasn't changes since then except just one typo in comment.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/