From: Johann Höpfner <hoepf@cit.tum.de>
Subject: sys/net/pf_ioctl.c fix two MSAN findings
To: tech@openbsd.org
Date: Sat, 5 Sep 2026 00:22:09 +0200

Hello tech@

Please find below two kernel memory sanitizer findings, which I find to
be bugs, and their respective fixes.

First, the check for buffer exhaustion in pfi_get_ifaces is off-by-one.
This leaves the last part of the buffer uninitialized and copies back
foreign heap data.

The issue seems to be, that we increment n before the early-exit, so we
increment n once to many if we exit in the line the following diff
replaces. (n is assigned later to *size and used as length for copyout
in the caller). Fix by making sure, that in every loop iteration n is
incremented, iff data is copied.

Index: sys/net/pf_if.c
===================================================================
RCS file: /cvs/src/sys/net/pf_if.c,v
diff -u -p -r1.113 pf_if.c
--- sys/net/pf_if.c	21 Apr 2026 06:38:28 -0000	1.113
+++ sys/net/pf_if.c	1 Sep 2026 15:19:38 -0000
@@ -789,8 +789,9 @@ pfi_get_ifaces(const char *name, struct 
 	RB_FOREACH(p, pfi_ifhead, &pfi_ifs) {
 		if (pfi_skip_if(name, p))
 			continue;
-		if (*size <= ++n)
+		if (n >= *size)
 			break;
+		n++;
 		if (!p->pfik_tzero)
 			p->pfik_tzero = gettime();
 		memcpy(buf++, p, sizeof(*buf));

Next, these five sites lead indirectly to two strlcpy's calls (in
pfi_update_status, pfi_kif_find) without proper buffer termination.
KASAN shouldn't catch them usually, because stkbuf in sys_sysctl is
longer than the ioctl struct copied in.

In general, strlcpy with discarded return value seems like an
antipattern to me. These cases should probably use a function that does
not scan the source until the next null-byte, but returns as soon as the
destination is full. As far as I know this is exactly what linux has
strScpy for.

Index: sys/net/pf_ioctl.c
===================================================================
RCS file: /cvs/src/sys/net/pf_ioctl.c,v
diff -u -p -r1.433 pf_ioctl.c
--- sys/net/pf_ioctl.c	27 Jul 2026 19:02:48 -0000	1.433
+++ sys/net/pf_ioctl.c	1 Sep 2026 15:19:39 -0000
@@ -2892,6 +2892,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 	case DIOCSETSTATUSIF: {
 		struct pfioc_iface	*pi = (struct pfioc_iface *)addr;
 
+		pi->pfiio_name[sizeof(pi->pfiio_name)-1] = '\0';
 		NET_LOCK();
 		PF_LOCK();
 		if (pi->pfiio_name[0] == 0) {
@@ -2910,6 +2911,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 	case DIOCCLRSTATUS: {
 		struct pfioc_iface	*pi = (struct pfioc_iface *)addr;
 
+		pi->pfiio_name[sizeof(pi->pfiio_name)-1] = '\0';
 		NET_LOCK();
 		PF_LOCK();
 		/* if ifname is specified, clear counters there only */
@@ -3869,6 +3871,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 			goto fail;
 		}
 
+		io->pfiio_name[sizeof(io->pfiio_name)-1] = '\0';
 		if ((kif_buf = mallocarray(sizeof(*kif_buf), apfiio_size,
 		    M_PF, M_WAITOK|M_CANFAIL)) == NULL) {
 			error = EINVAL;
@@ -3895,6 +3898,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 			goto fail;
 		}
 
+		io->pfiio_name[sizeof(io->pfiio_name)-1] = '\0';
 		PF_LOCK();
 		error = pfi_set_flags(io->pfiio_name, io->pfiio_flags);
 		PF_UNLOCK();
@@ -3909,6 +3913,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a
 			goto fail;
 		}
 
+		io->pfiio_name[sizeof(io->pfiio_name)-1] = '\0';
 		PF_LOCK();
 		error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags);
 		PF_UNLOCK();


Best regards
Johann Höpfner