netdev - Re: [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with weird arguements

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190527122425.GQ2606@hirez.programming.kicks-ass.net>
Date:   Mon, 27 May 2019 14:24:25 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Rick Edgecombe <rick.p.edgecombe@...el.com>
Cc:     linux-kernel@...r.kernel.org, sparclinux@...r.kernel.org,
        linux-mm@...ck.org, netdev@...r.kernel.org, luto@...nel.org,
        dave.hansen@...el.com, namit@...are.com,
        Meelis Roos <mroos@...ux.ee>,
        "David S. Miller" <davem@...emloft.net>,
        Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v4 2/2] vmalloc: Avoid rare case of flushing tlb with
 weird arguements

On Tue, May 21, 2019 at 01:51:37PM -0700, Rick Edgecombe wrote:
> In a rare case, flush_tlb_kernel_range() could be called with a start
> higher than the end. Most architectures should be fine with with this, but
> some may not like it, so avoid doing this.
> 
> In vm_remove_mappings(), in case page_address() returns 0 for all pages,
> _vm_unmap_aliases() will be called with start = ULONG_MAX, end = 0 and
> flush = 1.
> 
> If at the same time, the vmalloc purge operation is triggered by something
> else while the current operation is between remove_vm_area() and
> _vm_unmap_aliases(), then the vm mapping just removed will be already
> purged. In this case the call of vm_unmap_aliases() may not find any other
> mappings to flush and so end up flushing start = ULONG_MAX, end = 0. So
> only set flush = true if we find something in the direct mapping that we
> need to flush, and this way this can't happen.
> 
> Fixes: 868b104d7379 ("mm/vmalloc: Add flag for freeing of special permsissions")
> Cc: Meelis Roos <mroos@...ux.ee>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Dave Hansen <dave.hansen@...el.com>
> Cc: Borislav Petkov <bp@...en8.de>
> Cc: Andy Lutomirski <luto@...nel.org>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Nadav Amit <namit@...are.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> ---
>  mm/vmalloc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 836888ae01f6..537d1134b40e 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2125,6 +2125,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  	unsigned long addr = (unsigned long)area->addr;
>  	unsigned long start = ULONG_MAX, end = 0;
>  	int flush_reset = area->flags & VM_FLUSH_RESET_PERMS;
> +	int flush_dmap = 0;
>  	int i;
>  
>  	/*
> @@ -2163,6 +2164,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  		if (addr) {
>  			start = min(addr, start);
>  			end = max(addr + PAGE_SIZE, end);
> +			flush_dmap = 1;
>  		}
>  	}
>  
> @@ -2172,7 +2174,7 @@ static void vm_remove_mappings(struct vm_struct *area, int deallocate_pages)
>  	 * reset the direct map permissions to the default.
>  	 */
>  	set_area_direct_map(area, set_direct_map_invalid_noflush);
> -	_vm_unmap_aliases(start, end, 1);
> +	_vm_unmap_aliases(start, end, flush_dmap);
>  	set_area_direct_map(area, set_direct_map_default_noflush);
>  }

Hurmph.. another clue that this range flushing is crap I feel. The phys
addrs of the page array can be scattered all over the place, a range
doesn't properly represent things.

But yes, this seems like a minimal fix in spirit with the existing code.