lkml.org 
[lkml]   [2009]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86: ptrace debugreg checks rewrite

* Alexey Dobriyan <adobriyan@gmail.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@gmail.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


\
 
 \ /
  Last update: 2009-06-23 10:59    [W:0.043 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site