Re: [PATCH 0/1] (Was: Linux 3.11-rc4)

[Linus Torvalds <torvalds@linux-foundation.org>]

On Thu, Aug 8, 2013 at 8:41 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> On x86 execute breakpoints are only a single byte, which has to be
> the first byte of the instruction. IOW the hardware requires len = 1
> in dr7 or it doesn't work (iirc).
>
> But for some reason perf requires bp_len = sizeof(long), not 1. And
> note that it sets info->len = X86_BREAKPOINT_LEN_X. The comment says:
>
>         x86 inst breakpoints need to have a specific undefined len
>
> but despite its "special" name LEN_X is simply LEN_1, and other code
> relies on this fact.
>
> Now, ptrace correctly requires DR_LEN_1. So arch_bp_generic_fields()
> translates this into "gen_len = sizeof(long)" for validation.

Yeah, that just sounds insane. I suspect it's some misguided attempt
to be compatible either with some broken old version of perf. But if
so, I agree that the compatibility code should be elsewhere, and not
in "let's turn the _correct_ length of 1 into some random crap because
we screwed up elsewhere".

>> But the kernel address checking definitely needs to stay around for
>> security reasons.
>
> Sure. And btw it doesn't look right. I sent the patch below twice (iirc),
> perhaps I should resend it again.

Your patch looks correct.

That said,

> -       return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
> +       return (va >= TASK_SIZE) || ((va + len - 1) >= TASK_SIZE);

I'd much rather make this be more clearly about overflow, and write
this as something like

    last = va + len - 1;
    /* Check for overflow too */
    if (last < va || end >= TASK_SIZE)

because quite frankly, the "va >= TASK_SIZE" check is kind of insane.
It makes very little semantic sense. The rewritten test can be seen as
two independent tests that both make sense individually (the first
checks for overflow, the second checks that the range isn't in kernel
space).

In fact, the overflow check could/should even be done in generic code,
methinks. There's nothing architecture-specific about that.

                   Linus