NetBSD Problem Report #57683

From www@netbsd.org  Sun Nov  5 23:51:29 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 7A8741A9238
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  5 Nov 2023 23:51:29 +0000 (UTC)
Message-Id: <20231105235127.C6D451A9239@mollari.NetBSD.org>
Date: Sun,  5 Nov 2023 23:51:27 +0000 (UTC)
From: tnn@nygren.pp.se
Reply-To: tnn@nygren.pp.se
To: gnats-bugs@NetBSD.org
Subject: uvm_pglistalloc_s_ps() et al integer overflow
X-Send-Pr-Version: www-1.0

>Number:         57683
>Category:       kern
>Synopsis:       uvm_pglistalloc_s_ps() et al integer overflow
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 05 23:55:00 +0000 2023
>Closed-Date:    Fri Mar 01 13:42:20 +0000 2024
>Last-Modified:  Fri Mar 01 13:42:20 +0000 2024
>Originator:     Tobias Nygren
>Release:        10.99.10
>Organization:
>Environment:
aarch64
>Description:
When uvm_physseg_get_avail_end() is a large value (0x800000000 in my case),
some UVM functions can hit integer overflow when mapping device memory.
I see this when attempting to use nouveau on aarch64 (Ampere Altra),
hitting the assert here: https://github.com/NetBSD/src/blob/f0634c6a5ba141afed730885b76d7de8119e12c1/sys/uvm/uvm_pglist.c#L595

The attached patch fixes this problem and makes nouveau attach.

From inspection it looks like there are other functions that may have similar problems. At least uvm_pglistalloc_c_ps(), uvm_physseg_get_start_hint() and uvm_physseg_set_start_hint().
But problems will I think only occur here on systems with more than 16 TB of RAM.
>How-To-Repeat:

>Fix:
--- uvm_pglist.c	21 Dec 2021 08:27:49 -0000	1.90
+++ uvm_pglist.c	5 Nov 2023 23:44:19 -0000
@@ -523,7 +523,8 @@ static int
 uvm_pglistalloc_s_ps(uvm_physseg_t psi, int num, paddr_t low, paddr_t high,
     struct pglist *rlist)
 {
-	int todo, limit, candidate;
+	int todo;
+	long limit, candidate;
 	struct vm_page *pg;
 	bool second_pass;
 #ifdef PGALLOC_VERBOSE
@@ -546,9 +547,9 @@ uvm_pglistalloc_s_ps(uvm_physseg_t psi,
 		return -1;

 	todo = num;
-	candidate = uimax(low, uvm_physseg_get_avail_start(psi) +
+	candidate = ulmax(low, uvm_physseg_get_avail_start(psi) +
 	    uvm_physseg_get_start_hint(psi));
-	limit = uimin(high, uvm_physseg_get_avail_end(psi));
+	limit = ulmin(high, uvm_physseg_get_avail_end(psi));
 	pg = uvm_physseg_get_pg(psi, candidate - uvm_physseg_get_start(psi));
 	second_pass = false;

@@ -560,8 +561,8 @@ again:
 				break;
 			}
 			second_pass = true;
-			candidate = uimax(low, uvm_physseg_get_avail_start(psi));
-			limit = uimin(limit, uvm_physseg_get_avail_start(psi) +
+			candidate = ulmax(low, uvm_physseg_get_avail_start(psi));
+			limit = ulmin(limit, uvm_physseg_get_avail_start(psi) +
 			    uvm_physseg_get_start_hint(psi));
 			pg = uvm_physseg_get_pg(psi, candidate - uvm_physseg_get_start(psi));
 			goto again;
@@ -571,10 +572,10 @@ again:
 			paddr_t cidx = 0;
 			const uvm_physseg_t bank = uvm_physseg_find(candidate, &cidx);
 			KDASSERTMSG(bank == psi,
-			    "uvm_physseg_find(%#x) (%"PRIxPHYSSEG ") != psi %"PRIxPHYSSEG,
+			    "uvm_physseg_find(%#lx) (%"PRIxPHYSSEG ") != psi %"PRIxPHYSSEG,
 			     candidate, bank, psi);
 			KDASSERTMSG(cidx == candidate - uvm_physseg_get_start(psi),
-			    "uvm_physseg_find(%#x): %#"PRIxPADDR" != off %"PRIxPADDR,
+			    "uvm_physseg_find(%#lx): %#"PRIxPADDR" != off %"PRIxPADDR,
 			     candidate, cidx, candidate - uvm_physseg_get_start(psi));
 		}
 #endif
@@ -594,7 +595,7 @@ again:
 	uvm_physseg_set_start_hint(psi, candidate + 1 - uvm_physseg_get_avail_start(psi));
 	KASSERTMSG(uvm_physseg_get_start_hint(psi) <= uvm_physseg_get_avail_end(psi) -
 	    uvm_physseg_get_avail_start(psi),
-	    "%#x %u (%#x) <= %#"PRIxPADDR" - %#"PRIxPADDR" (%#"PRIxPADDR")",
+	    "%#lx %u (%#x) <= %#"PRIxPADDR" - %#"PRIxPADDR" (%#"PRIxPADDR")",
 	    candidate + 1,
 	    uvm_physseg_get_start_hint(psi),
 	    uvm_physseg_get_start_hint(psi),

>Release-Note:

>Audit-Trail:
From: matthew green <mrg@netbsd.org>
To: gnats-bugs@netbsd.org, tnn@nygren.pp.se
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org
Subject: re: kern/57683: uvm_pglistalloc_s_ps() et al integer overflow
Date: Mon, 06 Nov 2023 13:25:18 +1100

 > But problems will I think only occur here on systems with more than 16 T=
 B of RAM.

 we have all sorts of issues with anything beyond 8TB (signed 32 bit
 4K pages overflow) because we use "int" all over uvm to count pages.

 this is one of them, and if it's enough to fix your problem, then it
 is worth doing but i expect to see *all sorts of issues* in UVM when
 there's actually huge ram.

 this is not a small problem AFAICT.  getting to 16T by using unsigned
 page count would help, but it's not nearly enough for the future.


 .mrg.

From: Tobias Nygren <tnn@nygren.pp.se>
To: matthew green <mrg@netbsd.org>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/57683: uvm_pglistalloc_s_ps() et al integer overflow
Date: Mon, 6 Nov 2023 09:15:01 +0100

 On Mon, 06 Nov 2023 13:25:18 +1100
 matthew green <mrg@netbsd.org> wrote:

 > this is not a small problem AFAICT.  getting to 16T by using unsigned
 > page count would help, but it's not nearly enough for the future.

 The problem can be divided into supporting large physical
 address space and actually mapping (or avoiding to map) large areas
 with normal sized pages. Word-addressable CXL SSDs that map into
 physical address space are already announced products from the vendors.
 Those will need to use special apertures with large pages to keep the
 page count sane.

From: "Tobias Nygren" <tnn@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57683 CVS commit: src/sys/uvm
Date: Sat, 13 Jan 2024 09:44:42 +0000

 Module Name:	src
 Committed By:	tnn
 Date:		Sat Jan 13 09:44:42 UTC 2024

 Modified Files:
 	src/sys/uvm: uvm_pglist.c uvm_physseg.c uvm_physseg.h

 Log Message:
 uvm: change type of uvm_physseg.start_hint from u_int to u_long

 Avoids assertion failure in uvm_pglistalloc_s_ps() with large paddrs.
 PR kern/57683.


 To generate a diff of this commit:
 cvs rdiff -u -r1.90 -r1.91 src/sys/uvm/uvm_pglist.c
 cvs rdiff -u -r1.19 -r1.20 src/sys/uvm/uvm_physseg.c
 cvs rdiff -u -r1.8 -r1.9 src/sys/uvm/uvm_physseg.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->closed
State-Changed-By: tnn@NetBSD.org
State-Changed-When: Sat, 13 Jan 2024 09:45:57 +0000
State-Changed-Why:
fixed


State-Changed-From-To: closed->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 13 Jan 2024 17:35:39 +0000
State-Changed-Why:
This could stand pullup to netbsd-10, no?
(Feel free to re-close if it's inapplicable for some reason.)


From: matthew green <mrg@eterna23.net>
To: gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
    gnats-admin@netbsd.org, tnn@NetBSD.org, tnn@nygren.pp.se
Subject: re: kern/57683 (uvm_pglistalloc_s_ps() et al integer overflow)
Date: Sun, 14 Jan 2024 05:22:40 +1100

 tnn@NetBSD.org writes:
 > Synopsis: uvm_pglistalloc_s_ps() et al integer overflow
 >
 > State-Changed-From-To: open->closed
 > State-Changed-By: tnn@NetBSD.org
 > State-Changed-When: Sat, 13 Jan 2024 09:45:57 +0000
 > State-Changed-Why:
 > fixed

 pullups?


 .mrg.

From: Nick Hudson <nick.hudson@gmx.co.uk>
To: matthew green <mrg@eterna23.net>, gnats-bugs@netbsd.org
Cc: kern-bug-people@netbsd.org, netbsd-bugs@netbsd.org,
 gnats-admin@netbsd.org, tnn@NetBSD.org, tnn@nygren.pp.se
Subject: Re: kern/57683 (uvm_pglistalloc_s_ps() et al integer overflow)
Date: Sun, 14 Jan 2024 06:48:01 +0000

 On 13/01/2024 18:22, matthew green wrote:
 > tnn@NetBSD.org writes:
 >> Synopsis: uvm_pglistalloc_s_ps() et al integer overflow
 >>
 >> State-Changed-From-To: open->closed
 >> State-Changed-By: tnn@NetBSD.org
 >> State-Changed-When: Sat, 13 Jan 2024 09:45:57 +0000
 >> State-Changed-Why:
 >> fixed
 >
 > pullups?
 >
 >
 > .mrg.
 >
 it breaks builds as-is cf.

 https://releng.netbsd.org/builds/HEAD/202401132040Z/pmax.build.failed

 Nick

From: Tobias Nygren <tnn@NetBSD.org>
To: Nick Hudson <nick.hudson@gmx.co.uk>
Cc: gnats-bugs@netbsd.org
Subject: Re: kern/57683 (uvm_pglistalloc_s_ps() et al integer overflow)
Date: Sun, 14 Jan 2024 11:10:46 +0100

 On Sun, 14 Jan 2024 06:48:01 +0000
 Nick Hudson <nick.hudson@gmx.co.uk> wrote:

 > it breaks builds as-is cf.
 > 
 > https://releng.netbsd.org/builds/HEAD/202401132040Z/pmax.build.failed

 I'll have a look.
 I thought I had tested under #ifdef DEBUG but maybe I didn't.

From: Tobias Nygren <tnn@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: riastradh@NetBSD.org, Greg Oster <oster@netbsd.org>, matthew green
 <mrg@eterna23.net>
Subject: Re: kern/57683 (uvm_pglistalloc_s_ps() et al integer overflow)
Date: Mon, 15 Jan 2024 09:51:00 +0100

 On Sat, 13 Jan 2024 17:35:39 +0000 (UTC)
 riastradh@NetBSD.org wrote:

 > This could stand pullup to netbsd-10, no?
 > (Feel free to re-close if it's inapplicable for some reason.)

 I will send a request for netbsd-10 today. netbsd-9 diffs did not apply
 cleanly. There are some missing pullups causing conflicts for example
 PR 56074. Let me know if you think it is worth the effort to apply
 manually otherwise I will opt to skip it.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57683 CVS commit: [netbsd-10] src/sys/uvm
Date: Mon, 15 Jan 2024 14:15:55 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Jan 15 14:15:55 UTC 2024

 Modified Files:
 	src/sys/uvm [netbsd-10]: uvm_pglist.c uvm_physseg.c uvm_physseg.h

 Log Message:
 Pull up following revision(s) (requested by tnn in ticket #554):

 	sys/uvm/uvm_physseg.c: revision 1.20
 	sys/uvm/uvm_pglist.c: revision 1.91
 	sys/uvm/uvm_pglist.c: revision 1.92
 	sys/uvm/uvm_physseg.h: revision 1.9

 uvm: change type of uvm_physseg.start_hint from u_int to u_long
 Avoids assertion failure in uvm_pglistalloc_s_ps() with large paddrs.
 PR kern/57683.

 fix DEBUG build


 To generate a diff of this commit:
 cvs rdiff -u -r1.90 -r1.90.4.1 src/sys/uvm/uvm_pglist.c
 cvs rdiff -u -r1.17 -r1.17.20.1 src/sys/uvm/uvm_physseg.c
 cvs rdiff -u -r1.8 -r1.8.52.1 src/sys/uvm/uvm_physseg.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 01 Mar 2024 13:42:20 +0000
State-Changed-Why:
pullup-10 applied, probably not worth the trouble for pullup<=9


>Unformatted:

Home	PR Database Search