From: Vitaliy Makkoveev <mvs@openbsd.org>
Subject: Re: sys/dev/ic/lpt.c: race condition, kernel heap leaked to line printer
To: Johann Höpfner <hoepf@cit.tum.de>
Cc: tech@openbsd.org
Date: Wed, 8 Jul 2026 13:55:15 +0300

On Wed, Jul 08, 2026 at 12:11:40PM +0200, Johann Höpfner wrote:
> > lpt(4) is kernel locked, so it works not as you described.
> 
> Thanks for your reply. I read up on the locking code and now agree with
> your analysis and patches.
> 
> However with both of your patches we still get kernel heap thrown onto
> the parallel port and a verified out of bounds read. I think, I know
> why.
> 
> uiomove can block too. Thus in the current code Thread A could execute
> sc->sc_cp = sc->sc_inbuf in lptwrite; and then block in the uiomove. The
> sleep releases the kern_lock so that another thread could start
> executing lptwrite, then lptpushbytes and block while sc_cp > sc_inbuf.
> Thread A could wake back up and now only sets sc_count = n but does not
> reset sc_cp to the start of the buf leading to OOB reads.
> 

Yes, userland buffer should be pushed to swap to trigger this.

> Only the following, third patch on top of your changes finally resolves
> this thing for me. You likely know more than me about the order these
> things should happen for error handling, but we definitely cant sleep in
> between setting sc_cp and sc_count.
> 


> 
> Except for this info leak, this is a matter of opinion about the
> guarantees userland should be provided, on which I will gladly defer to
> you.
> 

In common case such guarantees are not possible unless you set single
thread.

Well, this is the final diff to fix this context switch fallout.

Index: sys/dev/ic/lpt.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/lpt.c,v
retrieving revision 1.17
diff -u -p -r1.17 lpt.c
--- sys/dev/ic/lpt.c	25 Jun 2025 20:28:09 -0000	1.17
+++ sys/dev/ic/lpt.c	8 Jul 2026 10:45:32 -0000
@@ -206,7 +206,7 @@ lptopen(dev_t dev, int flag, int mode, s
 	sc->sc_control = control;
 	bus_space_write_1(sc->sc_iot, sc->sc_ioh, lpt_control, control);
 
-	sc->sc_inbuf = malloc(LPT_BSIZE, M_DEVBUF, M_WAITOK);
+	sc->sc_inbuf = malloc(LPT_BSIZE, M_DEVBUF, M_WAITOK | M_ZERO);
 	sc->sc_count = 0;
 	sc->sc_state = LPT_OPEN;
 
@@ -309,6 +309,8 @@ lptpushbytes(struct lpt_softc *sc)
 						error = EIO;
 					if (error != EWOULDBLOCK)
 						return error;
+					if (sc->sc_count == 0)
+						return 0;
 				}
 				break;
 			}
@@ -360,10 +362,10 @@ lptwrite(dev_t dev, struct uio *uio, int
 	int error = 0;
 
 	while ((n = ulmin(LPT_BSIZE, uio->uio_resid)) != 0) {
-		sc->sc_cp = sc->sc_inbuf;
-		error = uiomove(sc->sc_cp, n, uio);
+		error = uiomove(sc->sc_inbuf, n, uio);
 		if (error != 0)
 			return error;
+		sc->sc_cp = sc->sc_inbuf;
 		sc->sc_count = n;
 		error = lptpushbytes(sc);
 		if (error) {