linux-kernel - Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set in __get_user

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: <Z+ySVprhAsXgL0bh@MiWiFi-R3L-srv>
Date: Wed, 2 Apr 2025 09:26:46 +0800
From: Baoquan He <bhe@...hat.com>
To: David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, yanjun.zhu@...ux.dev
Subject: Re: [PATCH v2 2/7] mm/gup: check if both GUP_GET and GUP_PIN are set
 in __get_user_pages() earlier

On 04/01/25 at 04:37pm, David Hildenbrand wrote:
> On 01.04.25 16:34, Baoquan He wrote:
> > On 04/01/25 at 10:02am, David Hildenbrand wrote:
> > > On 31.03.25 10:13, Baoquan He wrote:
...snip...
> > > We already have that check in is_valid_gup_args(), that catches all external
> > > users that could possibly mess this up.
> > 
> > Right.
> > 
> > > 
> > > So we can just convert that into a VM_WARN_ON_ONCE(), and while doing that,
> > > we should convert the VM_BUG_ON() above to a VM_WARN_ON_ONCE() as well.
> > 
> > Sounds great to me, will put below change into this patch of v3 as suggested.
> > Thanks.
> > 
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 9e4ed09c578b..d551da9549b1 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1427,10 +1427,10 @@ static long __get_user_pages(struct mm_struct *mm,
> >   	start = untagged_addr_remote(mm, start);
> > -	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
> > +	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
> >   	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
> > -	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> > +	if (VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
> >   			 (FOLL_PIN | FOLL_GET)))
> 
> That won't work (or shouldn't work :) ). Just drop the handling, it's a
> sanity check we don't expect to ever trigger, just like the old VM_BUG_ON
> above.

Ah, I misunderstood it. Will add below change into this patch of v3.

diff --git a/mm/gup.c b/mm/gup.c
index 9e4ed09c578b..a8a10a8ebf14 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1427,12 +1427,9 @@ static long __get_user_pages(struct mm_struct *mm,
 
 	start = untagged_addr_remote(mm, start);
 
-	VM_BUG_ON(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
-
-	/* FOLL_GET and FOLL_PIN are mutually exclusive. */
-	if (WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
-			 (FOLL_PIN | FOLL_GET)))
-		return -EINVAL;
+	VM_WARN_ON_ONCE(!!pages != !!(gup_flags & (FOLL_GET | FOLL_PIN)));
+	VM_WARN_ON_ONCE((gup_flags & (FOLL_PIN | FOLL_GET)) ==
+			(FOLL_PIN | FOLL_GET));
 
 	do {
 		struct page *page;