linux-kernel - Re: [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

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>] [day] [month] [year] [list]

Message-ID: <af484926-f784-c22e-74ed-bda227a64c31@suse.com>
Date:   Tue, 12 Dec 2017 14:14:54 +0100
From:   Juergen Gross <jgross@...e.com>
To:     Jan Beulich <JBeulich@...e.com>, Ingo Molnar <mingo@...nel.org>
Cc:     mingo@...e.hu, tglx@...utronix.de,
        xen-devel <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        linux-kernel@...r.kernel.org, hpa@...or.com
Subject: Re: [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

On 12/12/17 11:48, Jan Beulich wrote:
>>>> On 12.12.17 at 11:38, <mingo@...nel.org> wrote:
>> * Jan Beulich <JBeulich@...e.com> wrote:
>>> --- 4.15-rc3/arch/x86/xen/mmu_pv.c
>>> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c
>>> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p
>>>  	/* Graft it onto L4[511][510] */
>>>  	copy_page(level2_kernel_pgt, l2);
>>>  
>>> +	/* Zap execute permission from the ident map. Due to the sharing of
>>> +	 * L1 entries we need to do this in the L2. */
>>
>> please use the customary (multi-line) comment style:
>>
>>   /*
>>    * Comment .....
>>    * ...... goes here.
>>    */
>>
>> specified in Documentation/CodingStyle.
> 
> I would have but didn't because all other comments in this function
> use this (wrong) style. I've concluded that consistency is better
> here than matching the style doc. If the Xen maintainers tell me
> otherwise, I'll happily adjust the patch.

Yes, please use the correct style with new comments.

> 
>>> +	if (__supported_pte_mask & _PAGE_NX)
>>> +		for (i = 0; i < PTRS_PER_PMD; ++i) {
>>> +			if (pmd_none(level2_ident_pgt[i]))
>>> +				continue;
>>> +			level2_ident_pgt[i] =
>>> +				pmd_set_flags(level2_ident_pgt[i], _PAGE_NX);
>>
>> So the line break here is quite distracting, especially considering how similar it 
>> is to the alignment of the 'continue' statement. I.e. visually it looks like 
>> control flow alignment.
>>
>> Would be much better to just leave it a single page and ignore checkpatch 
>> here.
> 
> Again I'll wait to see what the Xen maintainers think. I too dislike
> line splits like this one, but the line ended up quite a bit too long,
> not just a character or two. I also wasn't sure whether splitting
> between the function arguments would be okay, leaving the first
> line just slightly too long.

That would result in a 80 character line, which IMHO is the best choice
here.


Juergen