All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address
@ 2016-01-29 11:36 ` Matt Fleming
  0 siblings, 0 replies; 4+ messages in thread
From: Matt Fleming @ 2016-01-29 11:36 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H . Peter Anvin
  Cc: linux-efi, linux-kernel, Matt Fleming, Borislav Petkov,
	Sai Praneeth Prakhya, stable,
	Viorel-Cătălin Răpițeanu

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.

Reported-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@gmail.com>
Tested-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@gmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
Cc: Borislav Petkov <bp@alien8.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
---
 arch/x86/mm/pageattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index fc6a4c8f6e2a..2440814b0069 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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++;
-- 
2.6.2

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address
@ 2016-01-29 11:36 ` Matt Fleming
  0 siblings, 0 replies; 4+ messages in thread
From: Matt Fleming @ 2016-01-29 11:36 UTC (permalink / raw)
  To: Ingo Molnar, Thomas Gleixner, H . Peter Anvin
  Cc: linux-efi-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matt Fleming,
	Borislav Petkov, Sai Praneeth Prakhya,
	stable-u79uwXL29TY76Z2rM5mHXA,
	Viorel-Cătălin Răpițeanu

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.

Reported-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Tested-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
Cc: Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>
---
 arch/x86/mm/pageattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index fc6a4c8f6e2a..2440814b0069 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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++;
-- 
2.6.2

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address
  2016-01-29 11:36 ` Matt Fleming
  (?)
@ 2016-01-29 12:05 ` Borislav Petkov
  -1 siblings, 0 replies; 4+ messages in thread
From: Borislav Petkov @ 2016-01-29 12:05 UTC (permalink / raw)
  To: Matt Fleming
  Cc: Ingo Molnar, Thomas Gleixner, H . Peter Anvin, linux-efi,
	linux-kernel, Sai Praneeth Prakhya, stable,
	Viorel-Cătălin Răpițeanu

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@gmail.com>
> Tested-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@gmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>

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

-- 
Regards/Gruss,
    Boris.

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tip:x86/urgent] x86/mm/pat: Avoid truncation when converting cpa->numpages to address
  2016-01-29 11:36 ` Matt Fleming
  (?)
  (?)
@ 2016-01-29 14:06 ` tip-bot for Matt Fleming
  -1 siblings, 0 replies; 4+ messages in thread
From: tip-bot for Matt Fleming @ 2016-01-29 14:06 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: tglx, sai.praneeth.prakhya, stable, matt, linux-kernel, hpa,
	rapiteanu.catalin, mingo, bp

Commit-ID:  742563777e8da62197d6cb4b99f4027f59454735
Gitweb:     http://git.kernel.org/tip/742563777e8da62197d6cb4b99f4027f59454735
Author:     Matt Fleming <matt@codeblueprint.co.uk>
AuthorDate: Fri, 29 Jan 2016 11:36:10 +0000
Committer:  Thomas Gleixner <tglx@linutronix.de>
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.

Reported-and-tested-by: Viorel-Cătălin Răpițeanu <rapiteanu.catalin@gmail.com>
Acked-by: Borislav Petkov <bp@alien8.de>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Matt Fleming <matt@codeblueprint.co.uk>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=110131
Link: http://lkml.kernel.org/r/1454067370-10374-1-git-send-email-matt@codeblueprint.co.uk
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/mm/pageattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index fc6a4c8..2440814 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -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++;

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-01-29 14:07 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-29 11:36 [PATCH] x86/mm/pageattr: Avoid truncation when converting cpa->numpages to address Matt Fleming
2016-01-29 11:36 ` Matt Fleming
2016-01-29 12:05 ` Borislav Petkov
2016-01-29 14:06 ` [tip:x86/urgent] x86/mm/pat: " tip-bot for Matt Fleming

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.