linux-kernel - Re: [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address

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: <20160129120521.GE10187@pd.tnic>
Date:	Fri, 29 Jan 2016 13:05:21 +0100
From:	Borislav Petkov <bp@...en8.de>
To:	Matt Fleming <matt@...eblueprint.co.uk>
Cc:	Ingo Molnar <mingo@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H . Peter Anvin" <hpa@...or.com>, linux-efi@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
	stable@...r.kernel.org,
	Viorel-Cătălin Răpițeanu 
	<rapiteanu.catalin@...il.com>
Subject: Re: [PATCH] x86/mm/pageattr: Avoid truncation when converting
 cpa->numpages to address

On Fri, Jan 29, 2016 at 11:36:10AM +0000, Matt Fleming wrote:
> There are a couple of nasty truncation bugs lurking in the pageattr
> code that can be triggered when mapping EFI regions, e.g. when we pass
> a cpa->pgd pointer. Because cpa->numpages is a 32-bit value, shifting
> left by PAGE_SHIFT will truncate the resultant address to 32-bits.
> 
> Viorel-Cătălin managed to trigger this bug on his Dell machine that
> provides a ~5GB EFI region which requires 1236992 pages to be mapped.

They're going to need all that?! Of course they do!

> When calling populate_pud() the end of the region gets calculated
> incorrectly in the following buggy expression,
> 
>   end = start + (cpa->numpages << PAGE_SHIFT);
>
> And only 188416 pages are mapped. Next, populate_pud() gets invoked
> for a second time because of the loop in __change_page_attr_set_clr(),
> only this time no pages get mapped because shifting the remaining
> number of pages (1048576) by PAGE_SHIFT is zero. At which point the
> loop in __change_page_attr_set_clr() spins forever because we fail to
> map progress.
> 
> Hitting this bug depends very much on the virtual address we pick to
> map the large region at and how many pages we map on the initial run
> through the loop. This explains why this issue was only recently hit
> with the introduction of commit
> 
>   a5caa209ba9c ("x86/efi: Fix boot crash by mapping EFI memmap entries bottom-up at runtime, instead of top-down")
> 
> It's interesting to note that safe uses of cpa->numpages do exist in
> the pageattr code. If instead of shifting ->numpages we multiply by
> PAGE_SIZE, no truncation occurs because PAGE_SIZE is a UL value, and
> so the result is unsigned long.
> 
> To avoid surprises when users try to convert very large cpa->numpages
> values to addresses, change the data type from 'int' to 'unsigned
> long', thereby making it suitable for shifting by PAGE_SHIFT without
> any type casting.
> 
> The alternative would be to make liberal use of casting, but that is
> far more likely to cause problems in the future when someone adds more
> code and fails to cast properly; this bug was difficult enough to
> track down in the first place.
> 
> Reported-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@...il.com>
> Tested-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@...il.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Matt Fleming <matt@...eblueprint.co.uk>

Acked-by: Borislav Petkov <bp@...e.de>

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.