Index | Thread | Search

From:
Johann Höpfner <hoepf@cit.tum.de>
Subject:
Re: sys/dev/ic/lpt.c: race condition, kernel heap leaked to line printer
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Tue, 7 Jul 2026 22:20:23 +0200

Download raw body.

Thread
> You are wrong. During the lptwrite() race, the winner lptpushbytes()
> or lptwrite() thread could set 'sc->sc_count' to 0.

A few things first. I have only tested this on multiprocessor systems so
that might be a hard requirement to even cause the bug. Secondly I only
became aware of this due to the ASAN violation which it triggers (and
would not if this was not an overflowed read) and have verified in a
debugger, that yes, it sometimes accesses hundreds of bytes above the
sc->sc_inbuf, and yes the data output to the parallel port matches
exactly what is stored above sc_inbuf in the heap.

> 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 15:06:05 -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;

This patch unfortunately does not help with my issue in any way, kernel
heap continues to be sent over the parallel port.

> This should not be the problem. In any case, the 'sc->sc_count' does not
> exceed LPT_BSIZE bytes, so we never read outside 'sc->sc_inbuf'.

My core disagrement with your assessment is sc->sc_count <= LPT_BSIZE is
not sufficient for preventing BOs. A stronger, necessary condition is
that sc->sc_cp + sc->sc_count <= sc->sc_inbuf + LPT_BSIZE

I might need to explain the behavior I observed in debugging in a bit
more detail. Hereafter follows one possible interleaving of two
concurrent lptwrite execution, causing an overflow.

T1: lptwrite        sc_cp    = sc_inbuf
                    sc_count = 1024
----------------------------------------------
T0: lptwrite:       sc_cp    = sc_inbuf
----------------------------------------------
T1: lptpushbytes    sc_cp    = sc_inbuf + 1
                    sc_count = 1023
----------------------------------------------
T0: lptwrite        sc_count = 1024
----------------------------------------------
T1: lptpushbytes    sc_cp    = sc_inbuf + 2
                    sc_count = 1023

and as easy as this we gain one extra byte (really any number we like
depending on the interleaving) of data being sent to the parallel port.


Honestly I do not see a world where it could be safe to have one thread
modifying sc->sc_cp while another is moving said pointer forward to copy
memory it points at. Why would we not want to serialize the lptwrite's?
Also, since sc_inbuf is shared, won't concurrent writes corrupt each
other's data in any case?

This whole driver seems to be designed without consideration of
multiprocessing. It is more than a decade my senior and was likely just
fine in a serial world. Fun fact: NetBSD has this exact bug too, as I
found out this afternoon.


> We do 'sc->sc_inbuf' allocation without M_ZERO flag, so after the device was
> opened, we could deliver some heap data until 'sc->sc_inbuf' filled. But
> this will be always the same data. Zeroing 'sc->sc_inbuf' should help.

In testing, yes there is a lot of repeated output but that should be
more or less expected as the sc_inbuf is fixed in memory for the
lifetime of the file descriptor. And no, the heap data is not only
delivered to the parallel port until the buffer is filled. I had the
reproducer running for an hour today, continously spewing heap.