linux-kernel - Re: [PATCH] mm: Fix __wp_page_copy_user fallback path for remote mm

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: <20241101120717.11db30a5abc6378da7910719@linux-foundation.org>
Date: Fri, 1 Nov 2024 12:07:17 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Asahi Lina <lina@...hilina.net>
Cc: Sergio Lopez Pascual <slp@...hat.com>, linux-mm@...ck.org,
 linux-kernel@...r.kernel.org, asahi@...ts.linux.dev, Jia He
 <justin.he@....com>, Yibo Cai <Yibo.Cai@....com>, Kirill A. Shutemov
 <kirill.shutemov@...ux.intel.com>, Catalin Marinas
 <catalin.marinas@....com>, Asahi Lina <lina@...hilina.net>, Sergio Lopez
 Pascual <slp@...hat.com>
Subject: Re: [PATCH] mm: Fix __wp_page_copy_user fallback path for remote mm

On Fri, 01 Nov 2024 21:08:02 +0900 Asahi Lina <lina@...hilina.net> wrote:

> If the source page is a PFN mapping, we copy back from userspace.
> However, if this fault is a remote access, we cannot use
> __copy_from_user_inatomic. Instead, use access_remote_vm() in this case.
> 
> Fixes WARN and incorrect zero-filling when writing to CoW mappings in
> a remote process, such as when using gdb on a binary present on a DAX
> filesystem.
>
> [  143.683782] ------------[ cut here ]------------
> [  143.683784] WARNING: CPU: 1 PID: 350 at mm/memory.c:2904 __wp_page_copy_user+0x120/0x2bc
>
> ...
>

Thanks.  I assume we should backport this into earlier kernels?

If so, a Fixes: target is desired, to tell people how far back in time
it should be ported.  I think it's

83d116c53058 ("mm: fix double page fault on arm64 if PTE_AF is cleared").


> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3081,13 +3081,18 @@ static inline int __wp_page_copy_user(struct page *dst, struct page *src,
>  			update_mmu_cache_range(vmf, vma, addr, vmf->pte, 1);
>  	}
>  
> +	/* If the mm is a remote mm, copy in the page using access_remote_vm() */
> +	if (current->mm != mm) {
> +		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr, PAGE_SIZE, 0) != PAGE_SIZE)
> +			goto warn;
> +	}
>  	/*
>  	 * This really shouldn't fail, because the page is there
>  	 * in the page tables. But it might just be unreadable,
>  	 * in which case we just give up and fill the result with
>  	 * zeroes.
>  	 */
> -	if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
> +	else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
>  		if (vmf->pte)
>  			goto warn;
>  

The coding style ends up being unconventional.  I made these changes:

--- a/mm/memory.c~mm-fix-__wp_page_copy_user-fallback-path-for-remote-mm-fix
+++ a/mm/memory.c
@@ -3081,18 +3081,20 @@ static inline int __wp_page_copy_user(st
 			update_mmu_cache_range(vmf, vma, addr, vmf->pte, 1);
 	}
 
-	/* If the mm is a remote mm, copy in the page using access_remote_vm() */
-	if (current->mm != mm) {
-		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr, PAGE_SIZE, 0) != PAGE_SIZE)
-			goto warn;
-	}
 	/*
-	 * This really shouldn't fail, because the page is there
-	 * in the page tables. But it might just be unreadable,
-	 * in which case we just give up and fill the result with
-	 * zeroes.
+	 * If the mm is a remote mm, copy in the page using access_remote_vm()
 	 */
-	else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
+	if (current->mm != mm) {
+		if (access_remote_vm(mm, (unsigned long)uaddr, kaddr,
+				     PAGE_SIZE, 0) != PAGE_SIZE)
+			goto warn;
+	} else if (__copy_from_user_inatomic(kaddr, uaddr, PAGE_SIZE)) {
+		/*
+		 * This really shouldn't fail, because the page is there
+		 * in the page tables. But it might just be unreadable,
+		 * in which case we just give up and fill the result with
+		 * zeroes.
+		 */
 		if (vmf->pte)
 			goto warn;
 
_

I'll queue this for testing and shall await further review.