Re: [PATCH 1/4] fs: tftp: prevent packet buffer overflow from long filenames

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

On 4/2/26 9:21 AM, Sascha Hauer wrote:
> tftp_send() constructs RRQ/WRQ packets by sprintf'ing the filename and
> TFTP options directly into the packet buffer with no bounds checking.
> The packet buffer is PKTSIZE (1536) bytes with 42 bytes of headers,
> leaving 1494 bytes of UDP payload. A filename longer than ~1418 bytes
> overflows the packet buffer, corrupting adjacent heap memory.
> 
> Replace sprintf with snprintf, tracking available buffer space and
> returning -ENAMETOOLONG if the packet would exceed the buffer.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  fs/tftp.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/tftp.c b/fs/tftp.c
> index a454306b4b..1b15bc18e7 100644
> --- a/fs/tftp.c
> +++ b/fs/tftp.c
> @@ -79,6 +79,10 @@
>  /* allocate this number of blocks more than needed in the fifo */
>  #define TFTP_EXTRA_BLOCKS	2
>  
> +/* Maximum UDP payload that fits in a PKTSIZE packet buffer */
> +#define TFTP_MAX_UDP_PAYLOAD	(PKTSIZE - ETHER_HDR_SIZE - \
> +				 sizeof(struct iphdr) - sizeof(struct udphdr))
> +
>  /* marker for an emtpy 'tftp_cache' */
>  #define TFTP_CACHE_NO_ID	(-1)
>  
> @@ -218,7 +222,9 @@ static int tftp_send(struct file_priv *priv)
>  
>  	switch (priv->state) {
>  	case STATE_RRQ:
> -	case STATE_WRQ:
> +	case STATE_WRQ: {
> +		int room, n;
> +
>  		if (priv->push || priv->is_getattr)
>  			/* atm, windowsize is supported only for RRQ and there
>  			   is no need to request a full window when we are
> @@ -235,7 +241,9 @@ static int tftp_send(struct file_priv *priv)
>  		else
>  			*s++ = htons(TFTP_WRQ);
>  		pkt = (unsigned char *)s;
> -		pkt += sprintf((unsigned char *)pkt,
> +		room = TFTP_MAX_UDP_PAYLOAD - (pkt - xp);
> +
> +		n = snprintf(pkt, room,
>  				"%s%c"
>  				"octet%c"
>  				"timeout%c"
> @@ -250,24 +258,38 @@ static int tftp_send(struct file_priv *priv)
>  				/* use only a minimal blksize for getattr
>  				   operations, */
>  				priv->is_getattr ? TFTP_BLOCK_SIZE : TFTP_MTU_SIZE);
> -		pkt++;
> +		if (n >= room)
> +			return -ENAMETOOLONG;
> +		pkt += n + 1;
> +		room -= n + 1;
>  
> -		if (!priv->push)
> +		if (!priv->push) {
>  			/* we do not know the filesize in WRQ requests and
>  			   'priv->filesize' will always be zero */
> -			pkt += sprintf((unsigned char *)pkt,
> +			n = snprintf(pkt, room,
>  				       "tsize%c%lld%c",
>  				       '\0', priv->filesize,
>  				       '\0');
> +			if (n >= room)
> +				return -ENAMETOOLONG;
> +			pkt += n;
> +			room -= n;
> +		}
>  
> -		if (window_size > 1)
> -			pkt += sprintf((unsigned char *)pkt,
> +		if (window_size > 1) {
> +			n = snprintf(pkt, room,
>  				       "windowsize%c%u%c",
>  				       '\0', window_size,
>  				       '\0');
> +			if (n >= room)
> +				return -ENAMETOOLONG;
> +			pkt += n;
> +			room -= n;
> +		}
>  
>  		len = pkt - xp;
>  		break;
> +	}
>  
>  	case STATE_RDATA:
>  		xp = pkt;
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |