From mboxrd@z Thu Jan 1 00:00:00 1970 From: Geert Uytterhoeven Subject: Re: [PATCH V11 3/5] printk: hash addresses printed with %p Date: Tue, 5 Dec 2017 23:57:40 +0100 Message-ID: References: <1511921105-3647-1-git-send-email-me@tobin.cc> <1511921105-3647-4-git-send-email-me@tobin.cc> <20171205204402.GD11064@eros> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Cc: kernel-hardening@lists.openwall.com, Linus Torvalds , "Jason A. Donenfeld" , "Theodore Ts'o" , Kees Cook , Paolo Bonzini , Tycho Andersen , "Roberts, William C" , Tejun Heo , Jordan Glover , Greg KH , Petr Mladek , Joe Perches , Ian Campbell , Sergey Senozhatsky , Catalin Marinas , Will Deacon , Steven Rostedt , Chris Fries , Dave Weinstein To: "Tobin C. Harding" Return-path: Received: from mail-qt0-f195.google.com ([209.85.216.195]:37189 "EHLO mail-qt0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752515AbdLEW5m (ORCPT ); Tue, 5 Dec 2017 17:57:42 -0500 In-Reply-To: <20171205204402.GD11064@eros> Sender: netdev-owner@vger.kernel.org List-ID: Hi Tobin, On Tue, Dec 5, 2017 at 9:44 PM, Tobin C. Harding wrote: > On Tue, Dec 05, 2017 at 09:20:57PM +0100, Geert Uytterhoeven wrote: >> On Wed, Nov 29, 2017 at 3:05 AM, Tobin C. Harding wrote: >> > Currently there exist approximately 14 000 places in the kernel where >> > addresses are being printed using an unadorned %p. This potentially >> > leaks sensitive information regarding the Kernel layout in memory. Many >> > of these calls are stale, instead of fixing every call lets hash the >> > address by default before printing. This will of course break some >> > users, forcing code printing needed addresses to be updated. >> > >> > Code that _really_ needs the address will soon be able to use the new >> > printk specifier %px to print the address. >> >> > --- a/lib/vsprintf.c >> > +++ b/lib/vsprintf.c >> >> > +/* Maps a pointer to a 32 bit unique identifier. */ >> > +static char *ptr_to_id(char *buf, char *end, void *ptr, struct printf_spec spec) >> > +{ >> > + unsigned long hashval; >> > + const int default_width = 2 * sizeof(ptr); >> > + >> > + if (unlikely(!have_filled_random_ptr_key)) { >> > + spec.field_width = default_width; >> > + /* string length must be less than default_width */ >> > + return string(buf, end, "(ptrval)", spec); >> > + } >> > + >> > +#ifdef CONFIG_64BIT >> > + hashval = (unsigned long)siphash_1u64((u64)ptr, &ptr_key); >> > + /* >> > + * Mask off the first 32 bits, this makes explicit that we have >> > + * modified the address (and 32 bits is plenty for a unique ID). >> > + */ >> > + hashval = hashval & 0xffffffff; >> > +#else >> > + hashval = (unsigned long)siphash_1u32((u32)ptr, &ptr_key); >> > +#endif >> >> Would it make sense to keep the 3 lowest bits of the address? >> >> Currently printed pointers no longer have any correlation with the actual >> alignment in memory of the object, which is a typical cause of a class of bugs. > > We'd have to keep the lowest 4 since we are printing in hex, right? This > is easy enough to add. I wasn't the architect behind the hashing but I > can do up a patch and see if anyone who knows crypto objects. Lowest 3 is good enough for all natural types, up to long long. We may still receive complaints from people who care about seeing if a pointer is cacheline-aligned or not. Fixing that may need up to 7 bits, I'm afraid, which is a bit too much to give up. Gr{oetje,eeting}s, Geert