Re: [PATCH] x86: ptrace debugreg checks rewrite

[Ingo Molnar]

* Alexey Dobriyan <adobriyan@xxxxxxxxx> wrote:

> On Tue, Jun 23, 2009 at 10:55:50AM +0200, Ingo Molnar wrote:
> > 
> > * Alexey Dobriyan <adobriyan@xxxxxxxxx> 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@xxxxxxxxx>
> > 
> > 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.

The debug register abstraction patches have been in the works for a 
year.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/