| Message ID | 20200403163253.GU20730@hirez.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/vmalloc: Sanitize __get_vm_area() arguments | expand |
On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > __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) <peterz@infradead.org> > --- > mm/vmalloc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n > struct 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)); > } 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
On Fri, Apr 03, 2020 at 08:18:18PM +0200, Uladzislau Rezki wrote: > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > __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) <peterz@infradead.org> > > --- > > mm/vmalloc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > --- a/mm/vmalloc.c > > +++ b/mm/vmalloc.c > > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n > > struct 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)); > > } > 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*. > __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.
On Fri, Apr 03, 2020 at 08:53:00PM +0200, Peter Zijlstra wrote: > On Fri, Apr 03, 2020 at 08:18:18PM +0200, Uladzislau Rezki wrote: > > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > > > __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) <peterz@infradead.org> > > > --- > > > mm/vmalloc.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > --- a/mm/vmalloc.c > > > +++ b/mm/vmalloc.c > > > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n > > > struct 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)); > > > } > > 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*. > 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. > > __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
On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > __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. > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > mm/vmalloc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > --- a/mm/vmalloc.c > +++ b/mm/vmalloc.c > @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n > struct 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)); > } ---end quoted text---
On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote: > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > __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. I think I just sent you a patch along those lines ;-)
Hi Christoph, On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote: > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > __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. 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.
On Fri, Apr 17, 2020 at 03:57:35PM +0300, Sakari Ailus wrote: > Hi Christoph, > > On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote: > > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > > > __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. > > 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: https://lkml.kernel.org/r/20200414131348.444715-5-hch@lst.de
On Fri, Apr 17, 2020 at 03:14:24PM +0200, Peter Zijlstra wrote: > On Fri, Apr 17, 2020 at 03:57:35PM +0300, Sakari Ailus wrote: > > Hi Christoph, > > > > On Mon, Apr 06, 2020 at 06:01:55AM -0700, Christoph Hellwig wrote: > > > On Fri, Apr 03, 2020 at 06:32:53PM +0200, Peter Zijlstra wrote: > > > > > > > > __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. > > > > 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: > > https://lkml.kernel.org/r/20200414131348.444715-5-hch@lst.de Ah, thanks for pointing this out. Please ignore this then.
--- a/mm/vmalloc.c +++ b/mm/vmalloc.c @@ -2130,6 +2130,13 @@ static struct vm_struct *__get_vm_area_n struct 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)); }
__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) <peterz@infradead.org> --- mm/vmalloc.c | 7 +++++++ 1 file changed, 7 insertions(+)