Date:   Thu, 2 Dec 2021 15:17:04 +0100
From:   Marco Elver <elver@google.com>
To:     andrey.konovalov@linux.dev
Cc:     Alexander Potapenko <glider@google.com>,
        Vincenzo Frascino <vincenzo.frascino@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Peter Collingbourne <pcc@google.com>,
        Andrey Konovalov <andreyknvl@gmail.com>,
        Dmitry Vyukov <dvyukov@google.com>, kasan-dev@googlegroups.com,
        Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        Will Deacon <will@kernel.org>,
        linux-arm-kernel@lists.infradead.org,
        Evgenii Stepanov <eugenis@google.com>,
        linux-kernel@vger.kernel.org,
        Andrey Konovalov <andreyknvl@google.com>
Subject: Re: [PATCH 20/31] kasan, vmalloc: reset tags in vmalloc functions
Message-ID: <YajVYNBDOyI3hTx1@elver.google.com>
References: <cover.1638308023.git.andreyknvl@google.com>
 <f405e36b20bd5d79dffef3f70b523885dcc6b163.1638308023.git.andreyknvl@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f405e36b20bd5d79dffef3f70b523885dcc6b163.1638308023.git.andreyknvl@google.com>
User-Agent: Mutt/2.0.5 (2021-01-21)
Precedence: bulk

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
>