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: <20090623085550.GE14560@elte.hu>
Date:	Tue, 23 Jun 2009 10:55:50 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Alexey Dobriyan <adobriyan@...il.com>,
	Frédéric Weisbecker <fweisbec@...il.com>
Cc:	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


* 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.

Thanks,

	Ingo
--
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/