x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address
Subsystems (4)
PATCH 1/1 x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address
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. 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.
--- arch/x86/mm/pageattr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/pageattr.c +2 -2
@@ -33,7 +33,7 @@ struct cpa_data {pgd_t *pgd;pgprot_t mask_set;pgprot_t mask_clr; -int numpages; +unsigned long numpages;int flags;unsigned long pfn; unsigned force_split : 1;
@@ -1350,7 +1350,7 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias)* CPA operation. Either a large page has been * preserved or a single page update happened. */ -BUG_ON (cpa->numpages > numpages); +BUG_ON (cpa->numpages > numpages || !cpa->numpages); numpages -= cpa->numpages;if (cpa->flags & (CPA_PAGES_ARRAY |CPA_ARRAY )) cpa->curpage++;
Signature · 1 line
2.6.2
Quoted 7 lines · Matt Fleming
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->pgdpointer. Becausecpa->numpagesis a 32-bit value, shifting left byPAGE_SHIFTwill 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!
Quoted 34 lines
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) byPAGE_SHIFTis 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->numpagesdo exist in the pageattr code. If instead of shifting ->numpages we multiply byPAGE_SIZE, no truncation occurs becausePAGE_SIZEis a UL value, and so the result is unsigned long.To avoid surprises when users try to convert very large
cpa->numpagesvalues to addresses, change the data type from 'int' to 'unsigned long', thereby making it suitable for shifting byPAGE_SHIFTwithout 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.
Quoted 7 lines
Reported-by Viorel-Cătălin Răpițeanu <[email protected]>Cc Borislav Petkov <[email protected]>Cc Sai Praneeth Prakhya <[email protected]>Cc <[email protected]>Signed-off-by Matt Fleming <[email protected]>
Signature · 4 lines
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.tip-bot for Matt Fleming
merge bot[tip:x86/urgent] x86/mm/pat: Avoid truncation when converting cpa->numpages to address
In reply to Matt Flemingx86/mm/pat: Avoid truncation when converting cpa->numpages to address
Commit-ID: 742563777e8da62197d6cb4b99f4027f59454735 Gitweb: http://git.kernel.org/tip/742563777e8da62197d6cb4b99f4027f59454735 Author: Matt Fleming <[email protected]> AuthorDate: Fri, 29 Jan 2016 11:36:10 +0000 Committer: Thomas Gleixner <[email protected]> CommitDate: Fri, 29 Jan 2016 15:03:09 +0100
x86/mm/pat: Avoid truncation when converting cpa->numpages to address
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. 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.
--- arch/x86/mm/pageattr.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/x86/mm/pageattr.c +2 -2
@@ -33,7 +33,7 @@ struct cpa_data {pgd_t *pgd;pgprot_t mask_set;pgprot_t mask_clr; -int numpages; +unsigned long numpages;int flags;unsigned long pfn; unsigned force_split : 1;
@@ -1350,7 +1350,7 @@ static int __change_page_attr_set_clr(struct cpa_data *cpa, int checkalias)* CPA operation. Either a large page has been * preserved or a single page update happened. */ -BUG_ON (cpa->numpages > numpages); +BUG_ON (cpa->numpages > numpages || !cpa->numpages); numpages -= cpa->numpages;if (cpa->flags & (CPA_PAGES_ARRAY |CPA_ARRAY )) cpa->curpage++;