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.
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html