linux-kernel - Re: [PATCH v2 1/8] x86: vdso: Introduce asm/vdso/mman.h

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]

Message-ID: <ZvH0PdVy5jJN3VTt@zx2c4.com>
Date: Tue, 24 Sep 2024 01:05:33 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Vincenzo Frascino <vincenzo.frascino@....com>
Cc: linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	linux-mm@...ck.org, Andy Lutomirski <luto@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Michael Ellerman <mpe@...erman.id.au>,
	Nicholas Piggin <npiggin@...il.com>,
	Naveen N Rao <naveen@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	"H . Peter Anvin" <hpa@...or.com>, Theodore Ts'o <tytso@....edu>,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH v2 1/8] x86: vdso: Introduce asm/vdso/mman.h

Hi,

I have the feeling I said this in the last two revisions, but maybe I
just thought it or agreed with somebody else who typed it but never
typed it myself, so now I'm typing it in no uncertain terms.

On Mon, Sep 23, 2024 at 03:19:36PM +0100, Vincenzo Frascino wrote:
> +#define VDSO_MMAP_PROT	PROT_READ | PROT_WRITE
> +#define VDSO_MMAP_FLAGS	MAP_DROPPABLE | MAP_ANONYMOUS

No, absolutely not. This is nonsense. Those flags aren't "the vdso
flags" or something. The variable name makes no sense. Moving the
definition outside of getrandom.c like the next patch does also makes no
sense. Do not do this.

If you need to, for some reason, rename those symbols, then rename them
each to VDSO_MAP_ANONYMOUS or whatever, and then use those from within
getrandom.c so it remains as readable and reasonable as it currently is.

But under no circumstances should you move where this is expressed and
rename it something generic like "vdso flags", when it is not generic
but rather very specific to the function where it is currently used.
IOW, please take a look and try to understand the code that you're
touching when proposing changes like this.

Also, though, I don't quite see what this patch accomplishes. If you're
fine doing #include <notvdso/whatever.h> into here, importing this
header into vdso code will transitively include notvdso/whatever.h with
it. So in that case, either we can keep using MAP_ANONYMOUS and whatnot
in the original sane symbol names, or this approach isn't correct in the
first place.

Maybe what you want instead is a simpler vdso/whatever.h header that
just includes nonvdso/whatever.h, and then you let getrandom.c and other
things keep using the same symbols as they were using before.

Jason