mm/vmalloc: Sanitize __get_vm_area() arguments
Subsystems (2)
PATCH 1/1 mm/vmalloc: Sanitize __get_vm_area() arguments
__get_vm_area() is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.
(I noticed this when I managed to corrupt the kernel text by accident)
--- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)
mm/vmalloc.c +7 -0
@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_nstruct vm_struct *__get_vm_area (unsigned long size,unsigned long flags,unsigned long start,unsigned long end) { +/* +* Ensure callers stay in the vmalloc range. +*/ +if (WARN_ON (start <VMALLOC_START || start >VMALLOC_END || + end <VMALLOC_START || end >VMALLOC_END )) +return NULL ; +return __get_vm_area_node (size,1 , flags, start, end,NUMA_NO_NODE ,GFP_KERNEL ,__builtin_return_address (0 )); }
Uladzislau Rezki
maintainerQuoted 18 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.(I noticed this when I managed to corrupt the kernel text by accident)
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)mm/vmalloc.c +0
-0@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_nstruct vm_struct *__get_vm_area (unsigned long size,unsigned long flags,unsigned long start,unsigned long end) {
mm/vmalloc.c static struct vm_struct *__get_vm_area_n · line 2142
Quoted 10 lines
mm/vmalloc.c +7
-0+/* +* Ensure callers stay in the vmalloc range. +*/ +if (WARN_ON (start <VMALLOC_START || start >VMALLOC_END || + end <VMALLOC_START || end >VMALLOC_END )) +return NULL ; +return __get_vm_area_node (size,1 , flags, start, end,NUMA_NO_NODE ,GFP_KERNEL ,__builtin_return_address (0 )); }
Peter, could you please clarify what kind of issues you had and how you tested?
__get_vm_area() is not limited by allocating only with vmalloc space, it can use whole virtual address space/range, i.e. 1 - ULONG_MAX.
Though, i am not sure if there are users(who uses __get_vm_area()) which allocate outside of vmalloc address space.
--
Vlad Rezki
Quoted 23 lines · Uladzislau Rezki
Quoted 22 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.(I noticed this when I managed to corrupt the kernel text by accident)
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)mm/vmalloc.c +4
-0@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_nstruct vm_struct *__get_vm_area (unsigned long size,unsigned long flags,unsigned long start,unsigned long end) { +/* +* Ensure callers stay in the vmalloc range. +*/ + if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||
Quoted 8 lines
mm/vmalloc.c static struct vm_struct *__get_vm_area_n · line 2142
Quoted 6 lines
mm/vmalloc.c +3
-0+ end < VMALLOC_START || end > VMALLOC_END)) +return NULL ; +return __get_vm_area_node (size,1 , flags, start, end,NUMA_NO_NODE ,GFP_KERNEL ,__builtin_return_address (0 )); }Peter, could you please clarify what kind of issues you had and how you tested?
Well, I had a bug and corrupted text; but then I tested:
__get_vm_area(PAGE_SIZE, VM_ALLOC, __START_KERNEL_map, __START_KERNEL_map + KERNEL_IMAGE_SIZE);
and that *works*.
Quoted 2 lines
__get_vm_area()is not limited by allocating only with vmalloc space, it can use whole virtual address space/range, i.e. 1 -ULONG_MAX.
Yeah, I know, I'm saying it perhaps should be, because not limiting it while exposing it to modules seems risky at best, downright dangerous if you consider map_vm_area() is also exported.
And while I know the machinery works for the complete virtual address space, architectures do set aside explicit VA ranges for specific purposes, we had better respect that, esp. for modules.
Uladzislau Rezki
maintainerQuoted 33 lines · Peter Zijlstra
Quoted 23 lines · Uladzislau Rezki
Quoted 22 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.(I noticed this when I managed to corrupt the kernel text by accident)
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)mm/vmalloc.c +4
-0@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_nstruct vm_struct *__get_vm_area (unsigned long size,unsigned long flags,unsigned long start,unsigned long end) { +/* +* Ensure callers stay in the vmalloc range. +*/ + if (WARN_ON(start < VMALLOC_START || start > VMALLOC_END ||Quoted 8 lines
mm/vmalloc.c static struct vm_struct *__get_vm_area_n · line 2142
Quoted 6 lines
mm/vmalloc.c +3
-0+ end < VMALLOC_START || end > VMALLOC_END)) +return NULL ; +return __get_vm_area_node (size,1 , flags, start, end,NUMA_NO_NODE ,GFP_KERNEL ,__builtin_return_address (0 )); }Peter, could you please clarify what kind of issues you had and how you tested?
Quoted 7 lines
Well, I had a bug and corrupted text; but then I tested:
__get_vm_area(PAGE_SIZE, VM_ALLOC, __START_KERNEL_map, __START_KERNEL_map + KERNEL_IMAGE_SIZE);and that *works*.
Do you mean that you corrupted "text" by calling __get_vm_area(...) with special parameters? If so could you please show how you used it.
Quoted 7 lines
Quoted 2 lines
__get_vm_area()is not limited by allocating only with vmalloc space, it can use whole virtual address space/range, i.e. 1 -ULONG_MAX.Yeah, I know, I'm saying it perhaps should be, because not limiting it while exposing it to modules seems risky at best, downright dangerous if you consider
map_vm_area()is also exported.
Doing it to secure modules, probably is OK, but modules can also be reside within vmalloc address space.
Thank you in advance!
--
Vlad Rezki
Quoted 6 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.(I noticed this when I managed to corrupt the kernel text by accident)
Maybe it is time to unexport it? There are only two users:
- staging/media/ipu3 really should be using vmap. And given that it is a staging driver it really doesn't matter anyway if we break it.
- pcmcia/electra_cf.c is actually using it for something that is not a vmalloc address. But it is so special that I think prohibiting to build it as module seems fine.
Quoted 12 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)mm/vmalloc.c +0
-0@@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_nstruct vm_struct *__get_vm_area (unsigned long size,unsigned long flags,unsigned long start,unsigned long end) {
mm/vmalloc.c static struct vm_struct *__get_vm_area_n · line 2142
Quoted 10 lines
mm/vmalloc.c +7
-0+/* +* Ensure callers stay in the vmalloc range. +*/ +if (WARN_ON (start <VMALLOC_START || start >VMALLOC_END || + end <VMALLOC_START || end >VMALLOC_END )) +return NULL ; +return __get_vm_area_node (size,1 , flags, start, end,NUMA_NO_NODE ,GFP_KERNEL ,__builtin_return_address (0 )); }
---end quoted text---
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.(I noticed this when I managed to corrupt the kernel text by accident)
Quoted 7 lines
Maybe it is time to unexport it? There are only two users:
- staging/media/ipu3 really should be using vmap. And given that it is a staging driver it really doesn't matter anyway if we break it.
-pcmcia/electra_cf.cis actually using it for something that is not a vmalloc address. But it is so special that I think prohibiting to build it as module seems fine.
I think I just sent you a patch along those lines ;-)
Hi Christoph,
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.
Quoted 6 lines
Quoted 1 line
(I noticed this when I managed to corrupt the kernel text by accident)
Maybe it is time to unexport it? There are only two users:
- staging/media/ipu3 really should be using vmap. And given that it is a staging driver it really doesn't matter anyway if we break it.
It's not very polite to suggest breaking other people's drivers for such a small matter, staging or not. That'd be bound to break kernel compilation for a lot of people, if for nothing else.
Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a patch to address it.
Signature · 3 lines
Regards, Sakari Ailus
Quoted 16 lines · Sakari Ailus
Hi Christoph,
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.Quoted 6 lines
Quoted 1 line
(I noticed this when I managed to corrupt the kernel text by accident)
Maybe it is time to unexport it? There are only two users:
- staging/media/ipu3 really should be using vmap. And given that it is a staging driver it really doesn't matter anyway if we break it.
Quoted 6 lines
It's not very polite to suggest breaking other people's drivers for such a small matter, staging or not. That'd be bound to break kernel compilation for a lot of people, if for nothing else.
Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a patch to address it.
Already done, see:
Quoted 21 lines · Peter Zijlstra
Quoted 20 lines · Sakari Ailus
Hi Christoph,
Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Peter Zijlstra
__get_vm_area()is an exported symbol, make sure the callers stay in the expected memory range. When calling this function with memory ranges outside of the VMALLOC range *bad* things can happen.Quoted 6 lines
Quoted 1 line
(I noticed this when I managed to corrupt the kernel text by accident)
Maybe it is time to unexport it? There are only two users:
- staging/media/ipu3 really should be using vmap. And given that it is a staging driver it really doesn't matter anyway if we break it.
It's not very polite to suggest breaking other people's drivers for such a small matter, staging or not. That'd be bound to break kernel compilation for a lot of people, if for nothing else.
Quoted 6 lines
Quoted 2 lines
Anyway, thanks for cc'ing me. I agree with suggestion and I'll submit a patch to address it.
Already done, see:
Ah, thanks for pointing this out. Please ignore this then.
Signature · 3 lines
Regards, Sakari Ailus