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

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]

Date:	Thu, 8 Aug 2013 09:25:54 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Grazvydas Ignotas <notasas@...il.com>,
	Felipe Contreras <felipe.contreras@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Ingo Molnar <mingo@...e.hu>,
	Denys Vlasenko <dvlasenk@...hat.com>
Subject: Re: [PATCH 0/1] (Was: Linux 3.11-rc4)

On Thu, Aug 8, 2013 at 8:41 AM, Oleg Nesterov <oleg@...hat.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
--
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/