From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Make sysctl_securelevel_int() mp-safe and unlock KERN_ALLOWDT
To: tech@openbsd.org
Date: Tue, 20 Aug 2024 17:22:22 +0300

Except the new value check, `securelevel' modification is identical to
sysctl_int_lower(). Introduce new sysctl_securelevel() to modify it
mp-safe.

We have many `securelevel' checks under kernel lock, so keep
KERN_SECURELVL locked, but start using atomic_load_int() for unlocked
read-only access to `securelevel' like within sysctl_securelevel_int().

Unlock KERN_ALLOWDT. `allowdt' is the atomically accessed integer used
only once in dtopen(). 

Index: sys/dev/dt/dt_dev.c
===================================================================
RCS file: /cvs/src/sys/dev/dt/dt_dev.c,v
diff -u -p -r1.34 dt_dev.c
--- sys/dev/dt/dt_dev.c	18 Aug 2024 08:23:58 -0000	1.34
+++ sys/dev/dt/dt_dev.c	20 Aug 2024 14:05:17 -0000
@@ -88,6 +88,7 @@
  * to keep track of enabled PCBs.
  *
  *  Locks used to protect struct members in this file:
+ *	a	atomic
  *	m	per-softc mutex
  *	K	kernel lock
  */
@@ -119,7 +120,7 @@ SIMPLEQ_HEAD(, dt_probe)	dt_probe_list;	
 struct rwlock			dt_lock = RWLOCK_INITIALIZER("dtlk");
 volatile uint32_t		dt_tracing = 0;	/* [K] # of processes tracing */
 
-int allowdt;
+int allowdt;					/* [a] */
 
 void	dtattach(struct device *, struct device *, void *);
 int	dtopen(dev_t, int, int, struct proc *);
@@ -162,7 +163,7 @@ dtopen(dev_t dev, int flags, int mode, s
 	struct dt_softc *sc;
 	int unit = minor(dev);
 
-	if (!allowdt)
+	if (atomic_load_int(&allowdt) == 0)
 		return EPERM;
 
 	sc = malloc(sizeof(*sc), M_DEVBUF, M_WAITOK|M_CANFAIL|M_ZERO);
Index: sys/kern/kern_sysctl.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_sysctl.c,v
diff -u -p -r1.442 kern_sysctl.c
--- sys/kern/kern_sysctl.c	20 Aug 2024 13:29:25 -0000	1.442
+++ sys/kern/kern_sysctl.c	20 Aug 2024 14:05:20 -0000
@@ -134,6 +134,7 @@ extern int autoconf_serial;
 
 int allowkmem;
 
+int sysctl_securelevel(void *, size_t *, void *, size_t, struct proc *);
 int sysctl_diskinit(int, struct proc *);
 int sysctl_proc_args(int *, u_int, void *, size_t *, struct proc *);
 int sysctl_proc_cwd(int *, u_int, void *, size_t *, struct proc *);
@@ -513,6 +514,11 @@ kern_sysctl(int *name, u_int namelen, vo
 		return (sysctl_rdstring(oldp, oldlenp, newp, version));
 	case KERN_NUMVNODES:  /* XXX numvnodes is a long */
 		return (sysctl_rdint(oldp, oldlenp, newp, numvnodes));
+#if NDT > 0
+	case KERN_ALLOWDT:
+		return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
+		    &allowdt));
+#endif
 	case KERN_HOSTID:
 		return (sysctl_int(oldp, oldlenp, newp, newlen, &hostid));
 	case KERN_CLOCKRATE:
@@ -596,26 +602,13 @@ int
 kern_sysctl_locked(int *name, u_int namelen, void *oldp, size_t *oldlenp,
     void *newp, size_t newlen, struct proc *p)
 {
-	int error, level, stackgap;
+	int error, stackgap;
 	dev_t dev;
 	extern int pool_debug;
 
 	switch (name[0]) {
 	case KERN_SECURELVL:
-		level = securelevel;
-		if ((error = sysctl_int(oldp, oldlenp, newp, newlen, &level)) ||
-		    newp == NULL)
-			return (error);
-		if ((securelevel > 0 || level < -1) &&
-		    level < securelevel && p->p_p->ps_pid != 1)
-			return (EPERM);
-		securelevel = level;
-		return (0);
-#if NDT > 0
-	case KERN_ALLOWDT:
-		return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
-		    &allowdt));
-#endif
+		return (sysctl_securelevel(oldp, oldlenp, newp, newlen, p));
 	case KERN_ALLOWKMEM:
 		return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen,
 		    &allowkmem));
@@ -1123,6 +1116,45 @@ sysctl_rdint(void *oldp, size_t *oldlenp
 	return (error);
 }
 
+int
+sysctl_securelevel(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
+    struct proc *p)
+{
+	int oldval, newval;
+	int error;
+
+	if (oldp && *oldlenp < sizeof(int))
+		return (ENOMEM);
+	if (newp && newlen != sizeof(int))
+		return (EINVAL);
+	*oldlenp = sizeof(int);
+
+	if (newp) {
+		if ((error = copyin(newp, &newval, sizeof(int))))
+			return (error);
+		do {
+			oldval = atomic_load_int(&securelevel);
+			if ((oldval > 0 || newval < -1) && newval < oldval &&
+			    p->p_p->ps_pid != 1)
+				return (EPERM);
+		} while (atomic_cas_uint(&securelevel, oldval, newval) !=
+		    oldval);
+
+		if (oldp) {
+			/* new value has been set although user gets error */
+			if ((error = copyout(&oldval, oldp, sizeof(int))))
+				return (error);
+		}
+	} else if (oldp) {
+		oldval = atomic_load_int(&securelevel);
+
+		if ((error = copyout(&oldval, oldp, sizeof(int))))
+			return (error);	
+	}
+
+	return (0);
+}
+
 /*
  * Selects between sysctl_rdint and sysctl_int according to securelevel.
  */
@@ -1130,7 +1162,7 @@ int
 sysctl_securelevel_int(void *oldp, size_t *oldlenp, void *newp, size_t newlen,
     int *valp)
 {
-	if (securelevel > 0)
+	if (atomic_load_int(&securelevel) > 0)
 		return (sysctl_rdint(oldp, oldlenp, newp, *valp));
 	return (sysctl_int(oldp, oldlenp, newp, newlen, valp));
 }