From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934905AbcDMOLs (ORCPT ); Wed, 13 Apr 2016 10:11:48 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:35546 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934385AbcDMOLq (ORCPT ); Wed, 13 Apr 2016 10:11:46 -0400 MIME-Version: 1.0 In-Reply-To: <20160413101950.GC9482@gmail.com> References: <1458631937-14593-1-git-send-email-bhe@redhat.com> <20160405015613.GA4654@x1.redhat.com> <20160413101950.GC9482@gmail.com> Date: Wed, 13 Apr 2016 07:11:44 -0700 X-Google-Sender-Auth: yRv2rylBI-u6cPW6mBnaqJh8pqk Message-ID: Subject: Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support From: Kees Cook To: Ingo Molnar Cc: Ingo Molnar , LKML , Baoquan He , Yinghai Lu , "H. Peter Anvin" , Borislav Petkov , Vivek Goyal , Andy Lutomirski , lasse.collin@tukaani.org, Andrew Morton , Dave Young , "kernel-hardening@lists.openwall.com" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> FWIW, I've also had this tree up in my git branches, and the 0day >> tester hasn't complained at all about it in the last two weeks. I'd >> really like to see this in -next to fix the >4G (mainly kexec) issues >> and get us to feature parity with the arm64 kASLR work (randomized >> virtual address). > > So I started applying the patches, started fixing up changelogs and gave up on > patch #3. > > Changelogs of such non-trivial code need to be proper English and need to be > understandable. > > For example patch #3 starts with: > >> Current z_extract_offset is calculated in boot/compressed/mkpiggy.c. The problem >> is in mkpiggy.c we don't know the detail of decompressor. Then we just get a >> rough z_extract_offset according to extra_bytes. As we know extra_bytes can only >> promise a safety margin when decompressing. In fact this brings some risks: > > Beyond the bad grammar of the _first word_ of the changelog, this is not a proper > high level description of the change. A _real_ high level description would be > something like: > > > Currently z_extract_offset is calculated during kernel build time. The problem > > with that method is that at this stage we don't yet know the decompression > > buffer sizes - we only know that during bootup. > > > > Effects of this are that when we calculate z_extract_offset during the build > > we don't know the precise decompression details, we'll only get a rough > > estimation of z_extract_offset. > > > > Instead of that we want to calculate it during bootup. > > etc. etc. - the whole series is _full_ of such crappy changelogs that make it > difficult for me and others to see whether the author actually _understands_ the > existing code or is hacking away on it. It's also much harder to review and > validate. > > This is totally unacceptable. > > Please make sure every changelog starts with a proper high level description that > tells the story and convinces the reader about what the problem is and what the > change should be. > > And part of that are the patch titles. Things like: > > Subject: [PATCH v3 03/19] x86, boot: Move z_extract_offset calculation to header.S > > are absolutely mindless titles. A better title would be: > > x86/boot: Calculate precise decompressor parameters during bootup, not build time > > ... or something like that. Even having read the changelog 3 times I'm unsure what > the change really is about. I'll rewrite all the changelogs and resend the series. Thanks taking a look! :) -Kees -- Kees Cook Chrome OS & Brillo Security From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <20160413101950.GC9482@gmail.com> References: <1458631937-14593-1-git-send-email-bhe@redhat.com> <20160405015613.GA4654@x1.redhat.com> <20160413101950.GC9482@gmail.com> Date: Wed, 13 Apr 2016 07:11:44 -0700 Message-ID: From: Kees Cook Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [PATCH v4 00/20] x86, boot: kaslr cleanup and 64bit kaslr support To: Ingo Molnar Cc: Ingo Molnar , LKML , Baoquan He , Yinghai Lu , "H. Peter Anvin" , Borislav Petkov , Vivek Goyal , Andy Lutomirski , lasse.collin@tukaani.org, Andrew Morton , Dave Young , "kernel-hardening@lists.openwall.com" List-ID: On Wed, Apr 13, 2016 at 3:19 AM, Ingo Molnar wrote: > > * Kees Cook wrote: > >> FWIW, I've also had this tree up in my git branches, and the 0day >> tester hasn't complained at all about it in the last two weeks. I'd >> really like to see this in -next to fix the >4G (mainly kexec) issues >> and get us to feature parity with the arm64 kASLR work (randomized >> virtual address). > > So I started applying the patches, started fixing up changelogs and gave up on > patch #3. > > Changelogs of such non-trivial code need to be proper English and need to be > understandable. > > For example patch #3 starts with: > >> Current z_extract_offset is calculated in boot/compressed/mkpiggy.c. The problem >> is in mkpiggy.c we don't know the detail of decompressor. Then we just get a >> rough z_extract_offset according to extra_bytes. As we know extra_bytes can only >> promise a safety margin when decompressing. In fact this brings some risks: > > Beyond the bad grammar of the _first word_ of the changelog, this is not a proper > high level description of the change. A _real_ high level description would be > something like: > > > Currently z_extract_offset is calculated during kernel build time. The problem > > with that method is that at this stage we don't yet know the decompression > > buffer sizes - we only know that during bootup. > > > > Effects of this are that when we calculate z_extract_offset during the build > > we don't know the precise decompression details, we'll only get a rough > > estimation of z_extract_offset. > > > > Instead of that we want to calculate it during bootup. > > etc. etc. - the whole series is _full_ of such crappy changelogs that make it > difficult for me and others to see whether the author actually _understands_ the > existing code or is hacking away on it. It's also much harder to review and > validate. > > This is totally unacceptable. > > Please make sure every changelog starts with a proper high level description that > tells the story and convinces the reader about what the problem is and what the > change should be. > > And part of that are the patch titles. Things like: > > Subject: [PATCH v3 03/19] x86, boot: Move z_extract_offset calculation to header.S > > are absolutely mindless titles. A better title would be: > > x86/boot: Calculate precise decompressor parameters during bootup, not build time > > ... or something like that. Even having read the changelog 3 times I'm unsure what > the change really is about. I'll rewrite all the changelogs and resend the series. Thanks taking a look! :) -Kees -- Kees Cook Chrome OS & Brillo Security