From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933255AbcFQIfn (ORCPT ); Fri, 17 Jun 2016 04:35:43 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:34182 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752337AbcFQIfk (ORCPT ); Fri, 17 Jun 2016 04:35:40 -0400 Date: Fri, 17 Jun 2016 10:35:35 +0200 From: Ingo Molnar To: Kees Cook Cc: Baoquan He , Borislav Petkov , Yinghai Lu , "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , "x86@kernel.org" , Andrew Morton , Josh Poimboeuf , Andrey Ryabinin , "H.J. Lu" , Dmitry Vyukov , LKML Subject: Re: [PATCH v9 3/5] x86/KASLR: Randomize virtual address separately Message-ID: <20160617083535.GA6996@gmail.com> References: <1464216334-17200-1-git-send-email-keescook@chromium.org> <1464216334-17200-4-git-send-email-keescook@chromium.org> <20160617082026.GA4791@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160617082026.GA4791@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Ingo Molnar wrote: > > * Kees Cook wrote: > > > -unsigned char *choose_random_location(unsigned long input, > > - unsigned long input_size, > > - unsigned long output, > > - unsigned long output_size) > > +void choose_random_location(unsigned long input, > > + unsigned long input_size, > > + unsigned long *output, > > + unsigned long output_size, > > + unsigned long *virt_addr) > > { > > - unsigned long choice = output; > > unsigned long random_addr; > > > > + /* By default, keep output position unchanged. */ > > + *virt_addr = *output; > > So I applied this, after fixing a conflict with a recent hibernation related > change, but it would be nice to further clean up the types in this file, in > particular could we please propagate 'const' for all input-only pointers? > > For example in the above function it would be obvious at a glance if it said > something like: > > void choose_random_location(unsigned long input, > unsigned long input_size, > const unsigned long *output, > unsigned long output_size, > unsigned long *virt_addr) > > when reading such a function prototype I can immediately tell: 'yeah, while it's > named "output", it's in fact a read-only input parameter - the _real_ output of > the function is 'virt_addr'.) Doh, so I managed to confuse myself by looking at the unpatched function only. This patch in fact starts writing to 'output': + /* Update the new physical address location. */ + if (*output != random_addr) { + add_identity_map(random_addr, output_size); + *output = random_addr; + } At which point 'output' cannot be const, and in fact it might be beneficial that 'virt_addr' is passed in by a pointer as well. The comment of the function definitely needs to be updated: * it takes the input and output pointers as 'unsigned long'. ... which is not true anymore. I also find the type flow and naming for the 'output' pointer very confusing. We have: asmlinkage __visible void *extract_kernel(void *rmode, memptr heap, unsigned char *input_data, unsigned long input_len, unsigned char *output, unsigned long output_len) ... choose_random_location((unsigned long)input_data, input_len, (unsigned long *)&output, max(output_len, kernel_total_size), &virt_addr); void choose_random_location(unsigned long input, unsigned long input_size, unsigned long *output, unsigned long output_size, unsigned long *virt_addr) ... *output = random_addr; it is very easy to confuse 'unsigned long *output' with the 'char *output' pointer to the output stream! But in reality this is a double pointer and we want to use it to change the pointer. So at minimum we should rename 'output' in choose_random_location() to something like 'output_ptr' - but even better would be to just preserve its natural type and use 'char * const *' and do a single type cast when setting it. Same goes for 'virt_addr'. Agreed? Thanks, Ingo