[PATCH] mm: fix overflow in vm_map_ram

[PATCH] mm: fix overflow in vm_map_ram

[Guillermo Julián Moreno]

When remapping pages accounting for 4G or more memory space, the
operation 'count << PAGE_SHIFT' overflows as it is performed on an
integer. Solution: cast before doing the bitshift.

Signed-off-by: Guillermo Julián <guillermo.julian@naudit.es>
---
mm/vmalloc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c  
index ae7d20b..97257e4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -1114,7 +1114,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
*/
void *vm_map_ram(struct page **pages, unsigned int count, int node, pgprot_t prot)
{
- unsigned long size = count << PAGE_SHIFT;
+ unsigned long size = ((unsigned long) count) << PAGE_SHIFT;
unsigned long addr;
void *mem;

@@ -1484,7 +1484,7 @@ static void __vunmap(const void *addr, int deallocate_pages)  
kfree(area);
return;
}
-
+
/**
* vfree - release memory allocated by vmalloc()
* @addr: memory base address
--
1.8.3.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix overflow in vm_map_ram

[Guillermo Julián Moreno]

On 20 April 2016 at 12:53:41, Guillermo Julián Moreno (guillermo.julian@naudit.es(mailto:guillermo.julian@naudit.es)) wrote:

>  
> When remapping pages accounting for 4G or more memory space, the
> operation 'count << PAGE_SHIFT' overflows as it is performed on an
> integer. Solution: cast before doing the bitshift.
>  
> Signed-off-by: Guillermo Julián  
> ---
> mm/vmalloc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>  
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index ae7d20b..97257e4 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1114,7 +1114,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
> */
> void *vm_map_ram(struct page **pages, unsigned int count, int node, pgprot_t prot)
> {
> - unsigned long size = count << PAGE_SHIFT;
> + unsigned long size = ((unsigned long) count) << PAGE_SHIFT;
> unsigned long addr;
> void *mem;
>  
> @@ -1484,7 +1484,7 @@ static void __vunmap(const void *addr, int deallocate_pages)
> kfree(area);
> return;
> }
> -
> +
> /**
> * vfree - release memory allocated by vmalloc()
> * @addr: memory base address
> --
> 1.8.3.1

Hello, has anyone taken a look at this patch?

Guillermo Julián



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix overflow in vm_map_ram

[Andrew Morton]

On Wed, 20 Apr 2016 12:53:33 +0200 Guillermo Juli__n Moreno <guillermo.julian@naudit.es> wrote:

> 
> When remapping pages accounting for 4G or more memory space, the
> operation 'count << PAGE_SHIFT' overflows as it is performed on an
> integer. Solution: cast before doing the bitshift.

Yup.

We need to work out which kernel versions to fix.  What are the runtime
effects of this?  Are there real drivers in the tree which actually map
more than 4G?

I fixed vm_unmap_ram() as well, but I didn't test it.  I wonder why you
missed that...

> diff --git a/mm/vmalloc.c b/mm/vmalloc.c  
> index ae7d20b..97257e4 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1114,7 +1114,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
> */
> void *vm_map_ram(struct page **pages, unsigned int count, int node, pgprot_t prot)
> {
> - unsigned long size = count << PAGE_SHIFT;
> + unsigned long size = ((unsigned long) count) << PAGE_SHIFT;
> unsigned long addr;
> void *mem;
> 

Your email client totally messes up the patches.  Please fix that for
next time.


From: Guillermo Juli_n Moreno <guillermo.julian@naudit.es>
Subject: mm: fix overflow in vm_map_ram()

When remapping pages accounting for 4G or more memory space, the
operation 'count << PAGE_SHIFT' overflows as it is performed on an
integer. Solution: cast before doing the bitshift.

[akpm@linux-foundation.org: fix vm_unmap_ram() also]
Link: http://lkml.kernel.org/r/etPan.57175fb3.7a271c6b.2bd@naudit.es
Signed-off-by: Guillermo Juli_n Moreno <guillermo.julian@naudit.es>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/vmalloc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram mm/vmalloc.c
--- a/mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram
+++ a/mm/vmalloc.c
@@ -1105,7 +1105,7 @@ EXPORT_SYMBOL_GPL(vm_unmap_aliases);
  */
 void vm_unmap_ram(const void *mem, unsigned int count)
 {
-	unsigned long size = count << PAGE_SHIFT;
+	unsigned long size = (unsigned long)count << PAGE_SHIFT;
 	unsigned long addr = (unsigned long)mem;
 
 	BUG_ON(!addr);
@@ -1140,7 +1140,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
  */
 void *vm_map_ram(struct page **pages, unsigned int count, int node, pgprot_t prot)
 {
-	unsigned long size = count << PAGE_SHIFT;
+	unsigned long size = (unsigned long)count << PAGE_SHIFT;
 	unsigned long addr;
 	void *mem;
 
_

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix overflow in vm_map_ram

[guillermo.julian]

El 2016-05-26 23:28, Andrew Morton escribiA3:
> On Wed, 20 Apr 2016 12:53:33 +0200 Guillermo Juli__n Moreno
> <guillermo.julian@naudit.es> wrote:
> 
>> 
>> When remapping pages accounting for 4G or more memory space, the
>> operation 'count << PAGE_SHIFT' overflows as it is performed on an
>> integer. Solution: cast before doing the bitshift.
> 
> Yup.
> 
> We need to work out which kernel versions to fix.  What are the runtime
> effects of this?  Are there real drivers in the tree which actually map
> more than 4G?

Looking at the references of vm_map_ram, it is only used in three 
drivers (XFS, v4l2-core and android/ion). However, in the vmap() code, 
the same bug is likely to occur (vmalloc.c:1557), and that function is 
more frequently used. But if it has gone unnoticed until now, most 
probably it isn't a critical issue (4G memory allocations are usually 
not needed. In fact this bug surfaced during a performance test in a 
modified driver, not in a regular configuration.

> 
> I fixed vm_unmap_ram() as well, but I didn't test it.  I wonder why you
> missed that...

The initial test didn't fail so I didn't notice the unmap was not 
working, so I completely forgot to check that function.

> 
>> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
>> index ae7d20b..97257e4 100644
>> --- a/mm/vmalloc.c
>> +++ b/mm/vmalloc.c
>> @@ -1114,7 +1114,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
>> */
>> void *vm_map_ram(struct page **pages, unsigned int count, int node, 
>> pgprot_t prot)
>> {
>> - unsigned long size = count << PAGE_SHIFT;
>> + unsigned long size = ((unsigned long) count) << PAGE_SHIFT;
>> unsigned long addr;
>> void *mem;
>> 
> 
> Your email client totally messes up the patches.  Please fix that for
> next time.

Sorry about that, I didn't notice it ate the tabs. I checked and this 
time it shouldn't happen.

> 
> 
> From: Guillermo Juli_n Moreno <guillermo.julian@naudit.es>
> Subject: mm: fix overflow in vm_map_ram()
> 
> When remapping pages accounting for 4G or more memory space, the
> operation 'count << PAGE_SHIFT' overflows as it is performed on an
> integer. Solution: cast before doing the bitshift.
> 
> [akpm@linux-foundation.org: fix vm_unmap_ram() also]
> Link: http://lkml.kernel.org/r/etPan.57175fb3.7a271c6b.2bd@naudit.es
> Signed-off-by: Guillermo Juli_n Moreno <guillermo.julian@naudit.es>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/vmalloc.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff -puN mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram mm/vmalloc.c
> --- a/mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram
> +++ a/mm/vmalloc.c
> @@ -1105,7 +1105,7 @@ EXPORT_SYMBOL_GPL(vm_unmap_aliases);
>   */
>  void vm_unmap_ram(const void *mem, unsigned int count)
>  {
> -	unsigned long size = count << PAGE_SHIFT;
> +	unsigned long size = (unsigned long)count << PAGE_SHIFT;
>  	unsigned long addr = (unsigned long)mem;
> 
>  	BUG_ON(!addr);
> @@ -1140,7 +1140,7 @@ EXPORT_SYMBOL(vm_unmap_ram);
>   */
>  void *vm_map_ram(struct page **pages, unsigned int count, int node,
> pgprot_t prot)
>  {
> -	unsigned long size = count << PAGE_SHIFT;
> +	unsigned long size = (unsigned long)count << PAGE_SHIFT;
>  	unsigned long addr;
>  	void *mem;
> 
> _

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: fix overflow in vm_map_ram

[Andrew Morton]

On Fri, 27 May 2016 10:25:59 +0200 "guillermo.julian" <guillermo.julian@naudit.es> wrote:

> El 2016-05-26 23:28, Andrew Morton escribi__:
> > On Wed, 20 Apr 2016 12:53:33 +0200 Guillermo Juli__n Moreno
> > <guillermo.julian@naudit.es> wrote:
> > 
> >> 
> >> When remapping pages accounting for 4G or more memory space, the
> >> operation 'count << PAGE_SHIFT' overflows as it is performed on an
> >> integer. Solution: cast before doing the bitshift.
> > 
> > Yup.
> > 
> > We need to work out which kernel versions to fix.  What are the runtime
> > effects of this?  Are there real drivers in the tree which actually map
> > more than 4G?
> 
> Looking at the references of vm_map_ram, it is only used in three 
> drivers (XFS, v4l2-core and android/ion). However, in the vmap() code, 
> the same bug is likely to occur (vmalloc.c:1557), and that function is 
> more frequently used. But if it has gone unnoticed until now, most 
> probably it isn't a critical issue (4G memory allocations are usually 
> not needed. In fact this bug surfaced during a performance test in a 
> modified driver, not in a regular configuration.

Yup.  I'll add this as well:

From: Andrew Morton <akpm@linux-foundation.org>
Subject: mm-fix-overflow-in-vm_map_ram-fix

fix vmap() as well, per Guillermo

Cc: Guillermo Juli_n Moreno <guillermo.julian@naudit.es>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/vmalloc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff -puN mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram-fix mm/vmalloc.c
--- a/mm/vmalloc.c~mm-fix-overflow-in-vm_map_ram-fix
+++ a/mm/vmalloc.c
@@ -1574,14 +1574,15 @@ void *vmap(struct page **pages, unsigned
 		unsigned long flags, pgprot_t prot)
 {
 	struct vm_struct *area;
+	unsigned long size;		/* In bytes */
 
 	might_sleep();
 
 	if (count > totalram_pages)
 		return NULL;
 
-	area = get_vm_area_caller((count << PAGE_SHIFT), flags,
-					__builtin_return_address(0));
+	size = (unsigned long)count << PAGE_SHIFT;
+	area = get_vm_area_caller(size, flags, __builtin_return_address(0));
 	if (!area)
 		return NULL;
 
_


I checked all other instances of "<< PAGE" in vmalloc.c and we're good.
Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>