linux-kernel - Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory region initial size

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: <20190406044348.GA14245@zn.tnic>
Date:   Sat, 6 Apr 2019 06:43:48 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Baoquan He <bhe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
        mingo@...nel.org, hpa@...or.com, kirill@...temov.name,
        keescook@...omium.org, yamada.masahiro@...ionext.com,
        dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
        thgarnie@...gle.com, mike.travis@....com, frank.ramsay@....com
Subject: Re: [PATCH 1/2] x86/mm/KASLR: Fix the wrong calculation of memory
 region initial size

On Sat, Apr 06, 2019 at 09:51:19AM +0800, Baoquan He wrote:
> It's KASLR happened in kernel_randomize_memory() of arch/x86/mm/kaslr.c .

What is "KASLR happened in"? This doesn't make any sense. When you look
at that function, there's a comment above it:

/* Initialize base and padding for each memory region randomized with KASLR */

Do you mean, that, per chance?

> In fact, I don't know how to call it. Previously, I wrote it as mm
> KASLR, to distinguish from KASLR during kernel decompression. Ingo
> blamed the name,

Of course he did, because it didn't make any sense to him either.

> so I changed it to memory region KASLR. Seems Thomas
> Garnier called it KASLR for kernel memory regions in his original KASLR
> adding patch. May I call it 'KASLR for kernel memory regions', or 'KASLR
> for memory regions'?

So you're fixing kaslr_regions[0].size_tb. It's base gets initialized to
page_offset_base.

Now, if you look at

Documentation/x86/x86_64/mm.txt

it says there:

 ffff888000000000 | -119.5  TB | ffffc87fffffffff |   64 TB | direct mapping of all physical memory (page_offset_base)

so that is the direct mapping memory region of all physical memory.

Now, you're apparently fixing its size.

Am I making sense and are you catching my drift?

You need to explain what you change in your commit messages in
*understandable* english so that reviewer/committer or even a person
who's not deeply involved in KASLR inner workings, can at least get an
idea about what the commit message is talking about.

If you come up with strange constructs like "memory region KASLR" or
"KASLR happened in" or "mm KASLR" which only make sense in your head,
you're not doing anyone any favour.

Commit messages need to be very understandable when someone is looking
at them months or even years from now. And you need to restrain yourself
when you write them. You will appreciate that the first time you have to
do git archeology, dig out an ancient commit and wonder why we did it
this way.

Because we didn't document as good in previous years and our commits
from the past suck big time.

Thanks!

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.