Re: [PATCH v2 2/3] net: tftp: verify the OACK packet is NUL terminated

[Jerome Forissier <jerome.forissier@arm.com>]

On 15/09/2026 22:52, Sriram Sriram wrote:
> From: Drew Kluemke <ankluemk@microsoft.com>
> 
> The OACK handler matches option names with strcasecmp() and parses the
> values with dectoul(). Both scan until a NUL byte, so a malformed or
> truncated OACK that contains no NUL within the received length makes
> them read past the end of the packet buffer.
> 
> RFC 2347 formats an OACK as a sequence of NUL terminated netascii
> strings, so the last byte of a well-formed packet is always a NUL.
> Check for it once on entry and drop the packet if it is missing, which
> keeps every strcasecmp() and dectoul() below in bounds without having to
> bound each one individually.
> 
> Signed-off-by: Drew Kluemke <ankluemk@microsoft.com>
> Signed-off-by: Sriram Sriram <sriramsriram@linux.microsoft.com>
> ---
> Changes in v2:
>  - Replaced the four bounded strncasecmp() comparisons with a single
>    check that the packet ends in a NUL, per Tom's review. RFC 2347
>    guarantees the trailing NUL, so one test covers every strcasecmp()
>    and, unlike v1, the dectoul() calls as well.
>  - Smaller: +16 bytes of .text on am335x_evm_defconfig, against +24 for
>    v1 (net/tftp.o 3222 unpatched, 3246 in v1, 3238 here).
>  - Retitled from "net: tftp: use bounded string compare for OACK option
>    parsing" to match the new approach.
> 
>  net/tftp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/tftp.c b/net/tftp.c
> index beb9d08f5a2..73b31b8357d 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -528,6 +528,13 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  #endif
>  
>  	case TFTP_OACK:
> +		/*
> +		 * RFC 2347 makes every option name and value a NUL terminated
> +		 * string, so a well-formed OACK always ends in a NUL. Checking
> +		 * that once keeps the option parsing below in bounds.
> +		 */
> +		if (!len || pkt[len - 1] != '\0')
> +			return;
>  		debug("Got OACK: ");
>  		for (i = 0; i < len; i++) {
>  			if (pkt[i] == '\0')

Reviewed-by: Jerome Forissier <jerome.forissier@arm.com>

Thanks,
-- 
Jerome