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
| ||
|
Date: Mon, 3 Sep 2018 23:57:03 +0200 (CEST) From: Thomas Gleixner <tglx@...utronix.de> To: Bin Yang <bin.yang@...el.com> cc: mingo@...nel.org, hpa@...or.com, x86@...nel.org, linux-kernel@...r.kernel.org, peterz@...radead.org, dave.hansen@...el.com, mark.gross@...el.com Subject: Re: [PATCH v3 1/5] x86/mm: avoid redundant checking if pgprot has no change On Tue, 21 Aug 2018, Bin Yang wrote: > --- a/arch/x86/mm/pageattr.c > +++ b/arch/x86/mm/pageattr.c > @@ -629,6 +629,22 @@ try_preserve_large_page(pte_t *kpte, unsigned long address, > new_prot = static_protections(req_prot, address, pfn); > > /* > + * The static_protections() is used to check specific protection flags > + * for certain areas of memory. The old pgprot should be checked already > + * when it was applied before. If it's not, then this is a bug in some > + * other code and needs to be fixed there. > + * > + * If new pgprot is same as old pgprot, return directly without any > + * additional checking. The following static_protections() checking is > + * pointless if pgprot has no change. It can avoid the redundant > + * checking and optimize the performance of large page split checking. > + */ > + if (pgprot_val(new_prot) == pgprot_val(old_prot)) { This is actually broken. Assume that for the start address: req_prot != old_prot and new_prot != req_prot and new_prot == old_prot and numpages > number_of_static_protected_pages(address) Then the new check will return with split = NO and the pages after the static protected area won't be updated -> FAIL! IOW, you partially reintroduce the bug which was fixed by adding this check loop. So this is a new optimization check which needs to be: if (pgprot_val(req_prot) == pgprot_val(old_prot)) and that check wants to go above: new_prot = static_protections(req_prot, address, pfn); Both under the assumption that old_prot is correct already. Now the question is whether this assumption can be made. The current code does that already today in case of page splits because it copies the existing pgprot of the large page unmodified over to the new split PTE page. IOW, if the current mapping is incorrect it will stay that way if it's not part of the actually modified range. I'm a bit worried about not having such a check, but if we add that then this should be done under a debug option for performance reasons. The last patch which does the overlap check is equally broken: + /* + * Ensure that the requested pgprot does not violate static protection + * requirements. + */ + new_prot = static_protections(req_prot, address, + numpages << PAGE_SHIFT, pfn); It expands new_prot to the whole range even if the protections only overlap. That should not happen in practice, but we have no checks for that at all. The whole thing needs way more thought in order not to (re)introduce subtle and hard to debug bugs. Thanks, tglx
Powered by blists - more mailing lists