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: Tue, 7 Jul 2026 09:51:33 +0000

On Tue, Jul 07, 2026 at 01:03:51AM +0200, Johann Höpfner wrote:
> Hello tech@
> 
> Again in testing my experimental kernel-address-sanitizer in syzkaller 
> another crash occurred that I believe to be a bug.
> 
> lptwrite, if executed on the same file concurrently has a race condition 
> wherein sc->sc_cp, sc->sc_count, etc can be (re)set by one thread entering
> lptwrite while another one executes lptpushbytes. This causes an out of 
> bounds read against sc->sc_inbuf as the loop keeps advancing the location
> that is read in the first which however reads an sc_count set by any 
> later thread to execute the first part of lptwrite.
> 
> Steps to reproduce:
> 
> run openbsd in a way that allows you to attach and monitor from a parallel
> port device, e.g. as under qemu using 
> 
> ```
> -chardev file,id=charptr0,path=log.txt \
> -device isa-parallel,chardev=charptr0
> ```
> 
> then run the attached program as root (adjust the device if no output is
> received by the virtual printer)
> 
> This bug has been quite unreliable for me, the output varying between 
> <1% to 70% leaked heap memory depending on the host system, the rest being
> the letter A as one would expected.
> 
> I would like to apologize in advance if reproducing this is painful.
> 
> ```c
> #define _GNU_SOURCE
> #include <stdio.h>
> #include <stdlib.h>
> #include <fcntl.h>
> #include <unistd.h>
> #include <pthread.h>
> #include <string.h>
> #include <errno.h>
> #include <time.h>
> #include <stdint.h>
> #include <sys/stat.h>
> 
> #define NUM_WRITERS 32
> #define ITERATIONS 1000
> 
> int fd;
> 
> void *writer_thread(void *arg) {
>     (void)arg;
>     char buf[4096];
>     srand((unsigned int)(uintptr_t)arg + time(NULL));
> 
>     for (int i = 0; i < ITERATIONS; i++) {
>         size_t len = (rand() % 1024) + 1;
>         memset(buf, 'A', len);
> 
>         ssize_t ret = write(fd, buf, len);
>         if (ret < 0) {
>             if (errno == EINTR || errno == EIO || errno == ENXIO || errno == EBUSY)
>                 continue;
>             perror("write");
>             break;
>         }
>     }
>     return NULL;
> }
> 
> int main(void) {
>     fd = open("/dev/lpa0", O_WRONLY);
>     if (fd < 0) {
>         perror("open");
>         return 1;
>     }
> 
>     pthread_t writers[NUM_WRITERS];
>     for (int i = 0; i < NUM_WRITERS; i++)
>         pthread_create(&writers[i], NULL, writer_thread, (void *)(uintptr_t)i);
> 
>     for (int i = 0; i < NUM_WRITERS; i++)
>         pthread_join(writers[i], NULL);
> 
>     return 0;
> }
> ```
> 
> We expect only the byte 'A' to be output in repeat but get 'A's 
> intermixed with leaked data from kernel heap allocations.
> 
> I propose the following patch serializing writes per port, unless someone 
> more familiar with the lpt driver can offer a less aggressive version.
> 

Hello Johann,

It seems we only need to check sc->sc_count against 0 after the sleep.
Otherwise the following "sc->sc_count--" produces integer overflow.

Does the diff below help?

Index: sys/dev/ic/lpt.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/lpt.c,v
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	7 Jul 2026 09:44:31 -0000
@@ -309,6 +309,8 @@ lptpushbytes(struct lpt_softc *sc)
 						error = EIO;
 					if (error != EWOULDBLOCK)
 						return error;
+					if (sc->sc_count == 0)
+						return 0;
 				}
 				break;
 			}