Re: sys/dev/ic/lpt.c: race condition, kernel heap leaked to line printer

Johann Höpfner <[email protected]>
Newsgroups gmane.os.openbsd.tech
Message-ID <[email protected]>
> 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.
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.