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

[Borislav Petkov]

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@xxxxxxxxx>
> Tested-by: Viorel-CÄtÄlin RÄpiÈeanu <rapiteanu.catalin@xxxxxxxxx>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
> Cc: Borislav Petkov <bp@xxxxxxxxx>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx>

Acked-by: Borislav Petkov <bp@xxxxxxx>

-- 
Regards/Gruss,
    Boris.

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