From: bluhm <bluhm@openbsd.org>
Date: Wed, 22 Dec 2021 22:20:13 +0000 (+0000)
Subject: While malloc sleeps, the disk list could change during sysctl.  Then
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=278923c07fda6048b6fcd284d05cda35cc047737;p=openbsd

While malloc sleeps, the disk list could change during sysctl.  Then
allocated memory could be too short for the list of disks.  Retry
allocating enough space until it did not change.
The disk list and duid memory are protected by kernel lock.  Use
asserts to mark this explicitly.
Reported-by: syzbot+807423f6868bbfb836bc@syzkaller.appspotmail.com
OK anton@ mpi@
---

diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c
index aacba022fd7..f3d3be011ed 100644
--- a/sys/kern/kern_sysctl.c
+++ b/sys/kern/kern_sysctl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: kern_sysctl.c,v 1.396 2021/10/30 23:24:48 deraadt Exp $	*/
+/*	$OpenBSD: kern_sysctl.c,v 1.397 2021/12/22 22:20:13 bluhm Exp $	*/
 /*	$NetBSD: kern_sysctl.c,v 1.17 1996/05/20 17:49:05 mrg Exp $	*/
 
 /*-
@@ -2132,12 +2132,19 @@ sysctl_diskinit(int update, struct proc *p)
 	struct diskstats *sdk;
 	struct disk *dk;
 	const char *duid;
-	int i, tlen, l;
+	int i, changed = 0;
+
+	KERNEL_ASSERT_LOCKED();
 
 	if ((i = rw_enter(&sysctl_disklock, RW_WRITE|RW_INTR)) != 0)
 		return i;
 
-	if (disk_change) {
+	/* Run in a loop, disks may change while malloc sleeps. */
+	while (disk_change) {
+		int tlen;
+
+		disk_change = 0;
+
 		for (dk = TAILQ_FIRST(&disklist), tlen = 0; dk;
 		    dk = TAILQ_NEXT(dk, dk_link)) {
 			if (dk->dk_name)
@@ -2146,10 +2153,12 @@ sysctl_diskinit(int update, struct proc *p)
 		}
 		tlen++;
 
-		if (disknames)
-			free(disknames, M_SYSCTL, disknameslen);
-		if (diskstats)
-			free(diskstats, M_SYSCTL, diskstatslen);
+		/*
+		 * The sysctl_disklock ensures that no other process can
+		 * allocate disknames and diskstats while our malloc sleeps.
+		 */
+		free(disknames, M_SYSCTL, disknameslen);
+		free(diskstats, M_SYSCTL, diskstatslen);
 		diskstats = NULL;
 		disknames = NULL;
 		diskstats = mallocarray(disk_count, sizeof(struct diskstats),
@@ -2158,13 +2167,18 @@ sysctl_diskinit(int update, struct proc *p)
 		disknames = malloc(tlen, M_SYSCTL, M_WAITOK|M_ZERO);
 		disknameslen = tlen;
 		disknames[0] = '\0';
+		changed = 1;
+	}
+
+	if (changed) {
+		int l;
 
 		for (dk = TAILQ_FIRST(&disklist), i = 0, l = 0; dk;
 		    dk = TAILQ_NEXT(dk, dk_link), i++) {
 			duid = NULL;
 			if (dk->dk_label && !duid_iszero(dk->dk_label->d_uid))
 				duid = duid_format(dk->dk_label->d_uid);
-			snprintf(disknames + l, tlen - l, "%s:%s,",
+			snprintf(disknames + l, disknameslen - l, "%s:%s,",
 			    dk->dk_name ? dk->dk_name : "",
 			    duid ? duid : "");
 			l += strlen(disknames + l);
@@ -2187,7 +2201,6 @@ sysctl_diskinit(int update, struct proc *p)
 		/* Eliminate trailing comma */
 		if (l != 0)
 			disknames[l - 1] = '\0';
-		disk_change = 0;
 	} else if (update) {
 		/* Just update, number of drives hasn't changed */
 		for (dk = TAILQ_FIRST(&disklist), i = 0; dk;
diff --git a/sys/kern/subr_disk.c b/sys/kern/subr_disk.c
index ecff6940caa..9673efe365f 100644
--- a/sys/kern/subr_disk.c
+++ b/sys/kern/subr_disk.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: subr_disk.c,v 1.246 2021/10/24 00:02:25 jsg Exp $	*/
+/*	$OpenBSD: subr_disk.c,v 1.247 2021/12/22 22:20:13 bluhm Exp $	*/
 /*	$NetBSD: subr_disk.c,v 1.17 1996/03/16 23:17:08 christos Exp $	*/
 
 /*
@@ -1045,6 +1045,8 @@ disk_attach(struct device *dv, struct disk *diskp)
 {
 	int majdev;
 
+	KERNEL_ASSERT_LOCKED();
+
 	if (!ISSET(diskp->dk_flags, DKF_CONSTRUCTED))
 		disk_construct(diskp);
 
@@ -1130,6 +1132,7 @@ done:
 void
 disk_detach(struct disk *diskp)
 {
+	KERNEL_ASSERT_LOCKED();
 
 	if (softraid_disk_attach)
 		softraid_disk_attach(diskp, -1);
@@ -1846,6 +1849,8 @@ duid_format(u_char *duid)
 {
 	static char duid_str[17];
 
+	KERNEL_ASSERT_LOCKED();
+
 	snprintf(duid_str, sizeof(duid_str),
 	    "%02x%02x%02x%02x%02x%02x%02x%02x",
 	    duid[0], duid[1], duid[2], duid[3],