linux-kernel - Re: [PATCH v2 1/2] x86/boot/64: Make level2_kernel

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: <20190924200440.GC8138@swahl-linux>
Date:   Tue, 24 Sep 2019 15:04:40 -0500
From:   Steve Wahl <steve.wahl@....com>
To:     Dave Hansen <dave.hansen@...el.com>
Cc:     Steve Wahl <steve.wahl@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
        Juergen Gross <jgross@...e.com>,
        "Kirill A. Shutemov" <kirill@...temov.name>,
        Brijesh Singh <brijesh.singh@....com>,
        Jordan Borgner <mail@...dan-borgner.de>,
        Feng Tang <feng.tang@...el.com>, linux-kernel@...r.kernel.org,
        Chao Fan <fanc.fnst@...fujitsu.com>,
        Zhenzhong Duan <zhenzhong.duan@...cle.com>,
        Baoquan He <bhe@...hat.com>, russ.anderson@....com,
        dimitri.sivanich@....com, mike.travis@....com
Subject: Re: [PATCH v2 1/2] x86/boot/64: Make level2_kernel_pgt pages invalid
 outside kernel area.

On Mon, Sep 23, 2019 at 02:19:46PM -0700, Dave Hansen wrote:
> On 9/23/19 11:15 AM, Steve Wahl wrote:
> >  	pmd = fixup_pointer(level2_kernel_pgt, physaddr);
> > -	for (i = 0; i < PTRS_PER_PMD; i++) {
> > +	for (i = 0; i < pmd_index((unsigned long)_text); i++)
> > +		pmd[i] &= ~_PAGE_PRESENT;
> > +
> > +	for (; i <= pmd_index((unsigned long)_end); i++)
> >  		if (pmd[i] & _PAGE_PRESENT)
> >  			pmd[i] += load_delta;
> > -	}
> > +
> > +	for (; i < PTRS_PER_PMD; i++)
> > +		pmd[i] &= ~_PAGE_PRESENT;
> 
> This is looking a bunch better.  The broken-up loop could probably use
> some comments, or you could combine it back to a single loop like this:
> 
> 	int text_start_pmd_index = pmd_index((unsigned long)_text);
> 	int text_end_pmd_index   = pmd_index((unsigned long)_end);
> 
> 	for (i = 0; i < PTRS_PER_PMD; i++) {
> 		if ((i < text_start_pmd_index) ||
> 		    (i > text_end_pmd_index)) {
> 			/* Unmap entries not mapping the kernel image */
> 			pmd[i] &= ~_PAGE_PRESENT;
> 		} else if (pmd[i] & _PAGE_PRESENT)
>  			pmd[i] += load_delta;
> 		}
> 	}

That's funny, I wrote it very close to that way initially, and
re-wrote it broken-up loop style because I thought it better conveyed
my intention.  (Mark pages before the kernel image as invalid, fixup
the pages that are in kernel image range, mark pages after the kernel
image as invalid.)

Given that, would you feel better with A) the way I have it, B) your
rewrite, or C) with an inline comment for each part of the loop:

	pmd = fixup_pointer(level2_kernel_pgt, physaddr);

	/* invalidate pages before the kernel image */
	for (i = 0; i < pmd_index((unsigned long)_text); i++)
		pmd[i] &= ~_PAGE_PRESENT;

	/* fixup pages that are part of the kernel image */
	for (; i <= pmd_index((unsigned long)_end); i++)
		if (pmd[i] & _PAGE_PRESENT)
			pmd[i] += load_delta;

	/* invalidate pages after the kernel image */
	for (; i < PTRS_PER_PMD; i++)
		pmd[i] &= ~_PAGE_PRESENT;

> Although I'd prefer it get commented or rewritten, it's passable like
> this, so:
> 
> Reviewed-by: Dave Hansen <dave.hansen@...ux.intel.com>

Thanks for your input!  

--> Steve Wahl

-- 
Steve Wahl, Hewlett Packard Enterprise