On Tue, Sep 15, 2026 at 11:44:40AM -0700, Sriram Sriram wrote: > option parsing > Reply-To: > Bcc: > Subject: Re: [PATCH 3/4] net: tftp: use bounded string compare for OACK > option parsing > Reply-To: > In-Reply-To: <20260910192538.GO1764417@bill-the-cat> > > On Thu, Sep 10, 2026 at 01:25:38PM -0600, Tom Rini wrote: > > On Wed, Sep 09, 2026 at 12:20:15PM -0700, Sriram Sriram wrote: > > > > > From: Drew Kluemke <[email protected]> > > > > > > The TFTP OACK handler matches option names in the received packet with > > > strcasecmp(), which scans until a NUL byte. A malformed or truncated > > > OACK packet may not contain a NUL within the received length, causing > > > an out-of-bounds read past the packet buffer. > > > > > > Replace each strcasecmp() with a strncasecmp() bounded to the option > > > name length plus an explicit check that the name is NUL-terminated at > > > the expected position. For the "windowsize" option, whose name and > > > value extend beyond the loop's "i + 8 < len" guard, add an explicit > > > "i + 11 < len" bound before the comparison. > > > > > > Signed-off-by: Drew Kluemke <[email protected]> > > > Signed-off-by: Sriram Sriram <[email protected]> > > > --- > > > net/tftp.c | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > Does the spec/RFC ensure that we're being given a NULL terminated > > string here? It certainly makes sense to be using strncasecmp here, > > but since we're a size constrained bootloader, we do need to find the > > right balance between defense-in-depth and potential exposure. > > Yes. RFC 2347 defines the OACK as > > +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+ > | opc | opt1 | 0 | value1 | 0 | optN | 0 | valueN | 0 | > +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+ > > so every option name and value is a NUL terminated netascii string, and > the last byte of a well-formed OACK is always a NUL. What we do not > have is any guarantee that the packet on the wire is well-formed -- > nothing in tftp_handler() checks that before the strcasecmp()s run. > > On exposure: it is real but small. strcasecmp() bails at the first > mismatching byte, so we only read past the packet when it ends part-way > through something that matches an option name, and the over-read is > then at most strlen("windowsize") + 1 == 11 bytes. Those bytes are > normally stale data in the same 1536 byte net_rx_packets[] buffer (the > receive buffers are one contiguous pool, net_rx_packets[i] = > net_tx_packet + (i + 1) * PKTSIZE_ALIGN), so we only leave the > allocation for a near-MTU OACK landing in the last rx buffer. > > The nastier case is the dectoul() right after a match: if the name and > its NUL end exactly at the end of the packet, dectoul() starts past the > end and runs until it hits a non-digit byte. v1 of this patch does not > address that at all. > > So I think the balance you are asking about argues for a smaller patch > than the one I sent. Since the RFC guarantees the trailing NUL, we can > check for it once instead of bounding four comparisons: > > case TFTP_OACK: > /* > * RFC 2347 requires every option name and value to be > * NUL terminated, so a well-formed OACK always ends in > * a NUL. Verifying that once keeps the option parsing > * below in bounds. > */ > if (!len || pkt[len - 1] != '\0') > return; > > That leaves the existing strcasecmp()/dectoul() code untouched, covers > the dectoul() calls as well as the name comparisons, and is cheaper. > net/tftp.o .text on am335x_evm_defconfig (arm-linux-gnueabi-, with > CONFIG_TFTP_TSIZE=y and CONFIG_TFTP_WINDOWSIZE=1): > > 3222 unpatched > 3246 v1, strncasecmp (+24) > 3238 trailing NUL check (+16) > > The one behaviour change is that a non-conforming OACK is dropped > rather than half parsed, so we fall back to the retry path instead of > ACKing with whatever the loop happened to pick up. > > I will send that as v2 unless you would rather not spend the 16 bytes > at all, in which case I will drop this patch -- the other three in the > series are independent of it.
Thanks for looking harder at this, yes, a 16 byte increase seems very reasonable. -- Tom
signature.asc
Description: PGP signature
