diff mbox series

[20/31] kasan, vmalloc: reset tags in vmalloc functions

Message ID f405e36b20bd5d79dffef3f70b523885dcc6b163.1638308023.git.andreyknvl@google.com (mailing list archive)
State New
Headers show
Series kasan, vmalloc, arm64: add vmalloc tagging support for SW/HW_TAGS | expand

Commit Message

andrey.konovalov@linux.dev Nov. 30, 2021, 10:07 p.m. UTC 
From: Andrey Konovalov <andreyknvl@google.com>

In preparation for adding vmalloc support to SW/HW_TAGS KASAN,
reset pointer tags in functions that use pointer values in
range checks.

vread() is a special case here. Resetting the pointer tag in its
prologue could technically lead to missing bad accesses to virtual
mappings in its implementation. However, vread() doesn't access the
virtual mappings cirectly. Instead, it recovers the physical address
via page_address(vmalloc_to_page()) and acceses that. And as
page_address() recovers the pointer tag, the accesses are checked.

Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
---
 mm/vmalloc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Marco Elver Dec. 2, 2021, 2:17 p.m. UTC | #1 
On Tue, Nov 30, 2021 at 11:07PM +0100, andrey.konovalov@linux.dev wrote:
> From: Andrey Konovalov <andreyknvl@google.com>
> 
> In preparation for adding vmalloc support to SW/HW_TAGS KASAN,
> reset pointer tags in functions that use pointer values in
> range checks.
> 
> vread() is a special case here. Resetting the pointer tag in its
> prologue could technically lead to missing bad accesses to virtual
> mappings in its implementation. However, vread() doesn't access the
> virtual mappings cirectly. Instead, it recovers the physical address

s/cirectly/directly/

But this paragraph is a little confusing, because first you point out
that vread() might miss bad accesses, but then say that it does checked
accesses. I think to avoid confusing the reader, maybe just say that
vread() is checked, but hypothetically, should its implementation change
to directly access addr, invalid accesses might be missed.

Did I get this right? Or am I still confused?

> via page_address(vmalloc_to_page()) and acceses that. And as
> page_address() recovers the pointer tag, the accesses are checked.
> 
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
>  mm/vmalloc.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index c5235e3e5857..a059b3100c0a 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -72,7 +72,7 @@ static const bool vmap_allow_huge = false;
>  
>  bool is_vmalloc_addr(const void *x)
>  {
> -	unsigned long addr = (unsigned long)x;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(x);
>  
>  	return addr >= VMALLOC_START && addr < VMALLOC_END;
>  }
> @@ -630,7 +630,7 @@ int is_vmalloc_or_module_addr(const void *x)
>  	 * just put it in the vmalloc space.
>  	 */
>  #if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
> -	unsigned long addr = (unsigned long)x;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(x);
>  	if (addr >= MODULES_VADDR && addr < MODULES_END)
>  		return 1;
>  #endif
> @@ -804,6 +804,8 @@ static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
>  	struct vmap_area *va = NULL;
>  	struct rb_node *n = vmap_area_root.rb_node;
>  
> +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> +
>  	while (n) {
>  		struct vmap_area *tmp;
>  
> @@ -825,6 +827,8 @@ static struct vmap_area *__find_vmap_area(unsigned long addr)
>  {
>  	struct rb_node *n = vmap_area_root.rb_node;
>  
> +	addr = (unsigned long)kasan_reset_tag((void *)addr);
> +
>  	while (n) {
>  		struct vmap_area *va;
>  
> @@ -2143,7 +2147,7 @@ EXPORT_SYMBOL_GPL(vm_unmap_aliases);
>  void vm_unmap_ram(const void *mem, unsigned int count)
>  {
>  	unsigned long size = (unsigned long)count << PAGE_SHIFT;
> -	unsigned long addr = (unsigned long)mem;
> +	unsigned long addr = (unsigned long)kasan_reset_tag(mem);
>  	struct vmap_area *va;
>  
>  	might_sleep();
> @@ -3361,6 +3365,8 @@ long vread(char *buf, char *addr, unsigned long count)
>  	unsigned long buflen = count;
>  	unsigned long n;
>  
> +	addr = kasan_reset_tag(addr);
> +
>  	/* Don't allow overflow */
>  	if ((unsigned long) addr + count < count)
>  		count = -(unsigned long) addr;
> -- 
> 2.25.1
>
Andrey Konovalov Dec. 6, 2021, 9:08 p.m. UTC | #2 
On Thu, Dec 2, 2021 at 3:17 PM Marco Elver <elver@google.com> wrote:
>
> On Tue, Nov 30, 2021 at 11:07PM +0100, andrey.konovalov@linux.dev wrote:
> > From: Andrey Konovalov <andreyknvl@google.com>
> >
> > In preparation for adding vmalloc support to SW/HW_TAGS KASAN,
> > reset pointer tags in functions that use pointer values in
> > range checks.
> >
> > vread() is a special case here. Resetting the pointer tag in its
> > prologue could technically lead to missing bad accesses to virtual
> > mappings in its implementation. However, vread() doesn't access the
> > virtual mappings cirectly. Instead, it recovers the physical address
>
> s/cirectly/directly/
>
> But this paragraph is a little confusing, because first you point out
> that vread() might miss bad accesses, but then say that it does checked
> accesses. I think to avoid confusing the reader, maybe just say that
> vread() is checked, but hypothetically, should its implementation change
> to directly access addr, invalid accesses might be missed.
>
> Did I get this right? Or am I still confused?

No, you got it right. Will reword in v2.

Thanks!
diff mbox series

Patch

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index c5235e3e5857..a059b3100c0a 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -72,7 +72,7 @@  static const bool vmap_allow_huge = false;
 
 bool is_vmalloc_addr(const void *x)
 {
-	unsigned long addr = (unsigned long)x;
+	unsigned long addr = (unsigned long)kasan_reset_tag(x);
 
 	return addr >= VMALLOC_START && addr < VMALLOC_END;
 }
@@ -630,7 +630,7 @@  int is_vmalloc_or_module_addr(const void *x)
 	 * just put it in the vmalloc space.
 	 */
 #if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
-	unsigned long addr = (unsigned long)x;
+	unsigned long addr = (unsigned long)kasan_reset_tag(x);
 	if (addr >= MODULES_VADDR && addr < MODULES_END)
 		return 1;
 #endif
@@ -804,6 +804,8 @@  static struct vmap_area *find_vmap_area_exceed_addr(unsigned long addr)
 	struct vmap_area *va = NULL;
 	struct rb_node *n = vmap_area_root.rb_node;
 
+	addr = (unsigned long)kasan_reset_tag((void *)addr);
+
 	while (n) {
 		struct vmap_area *tmp;
 
@@ -825,6 +827,8 @@  static struct vmap_area *__find_vmap_area(unsigned long addr)
 {
 	struct rb_node *n = vmap_area_root.rb_node;
 
+	addr = (unsigned long)kasan_reset_tag((void *)addr);
+
 	while (n) {
 		struct vmap_area *va;
 
@@ -2143,7 +2147,7 @@  EXPORT_SYMBOL_GPL(vm_unmap_aliases);
 void vm_unmap_ram(const void *mem, unsigned int count)
 {
 	unsigned long size = (unsigned long)count << PAGE_SHIFT;
-	unsigned long addr = (unsigned long)mem;
+	unsigned long addr = (unsigned long)kasan_reset_tag(mem);
 	struct vmap_area *va;
 
 	might_sleep();
@@ -3361,6 +3365,8 @@  long vread(char *buf, char *addr, unsigned long count)
 	unsigned long buflen = count;
 	unsigned long n;
 
+	addr = kasan_reset_tag(addr);
+
 	/* Don't allow overflow */
 	if ((unsigned long) addr + count < count)
 		count = -(unsigned long) addr;