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 18:54:30 +0200
From:	Frederic Weisbecker <fweisbec@...il.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Grazvydas Ignotas <notasas@...il.com>,
	Felipe Contreras <felipe.contreras@...il.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...e.hu>,
	Denys Vlasenko <dvlasenk@...hat.com>
Subject: Re: [PATCH 0/1] (Was: Linux 3.11-rc4)

On Thu, Aug 08, 2013 at 05:41:07PM +0200, Oleg Nesterov wrote:
> On 08/07, Linus Torvalds wrote:
> >
> > Now, I do agree that the debug registers are *much* less likely to
> > have those kinds of really subtle issues, so maybe relaxing some of
> > the tests might be reasonable. I'd be a bit nervous about it, but if
> > it's *only* the length/alignment, and Intel people can be convinced
> > that it doesn't result in any nasty undefined behavior (as long as the
> > address is in user space), maybe we could make that change just to
> > make it easier for Wine.
> 
> Oh, I do not know. And again, this way a user can't notice the problem
> if the arguments are wrong.
> 
> But personally I think it would be nice to cleanup the perf interface,
> although probably it is too later.
> 
> 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.
> 
> arch_build_bp_info() thinks that X86_BREAKPOINT_EXECUTE should have
> ->bp_len == sizeof(long), so we translate it back into LEN_1 internally.

I did this interface and I'm sorry about it.

This bp_len == sizeof(long) requirement comes from a very buggy conception
I had at the time I wrote that. I thought it would be pretty intuitive to
assume that instruction breakpoints should be the size of the instruction
itself as a generic interface for all archs. But at least x86 instructions
size aren't static. That sizeof(long) assumption just popped up from nowhere
at 5 am two years ago I guess :-(

And worse: I realized that mistake later but never moved it in the top of the
TODO-list pile because I had the feeling that nobody was using the perf breakpoint
interface anyway.

I'm all for fixing this. May be we can start by backporting a patch that
ignores the value of gen_len for instruction breakpoints in x86?

I don't know how other archs use it. I need to check. But this bp_len
should rather be used for range breakpoints on archs that support it. I
hope we can still reuse it if the damage of my initial misconception
isn't too widely expanded.

What do you think?

> 
> This looks confusing, imho. And imho X86_BREAKPOINT_LEN_X should die.

Yep.

Thanks.
--
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/