[PATCH v2 1/4] net: lwip: tftp: fix find_option()

[Jerome Forissier]

Hi Heinrich,

On 2/1/25 07:22, Heinrich Schuchardt wrote:
> Find_option() is used to retrieve the block size value in an option
> acknowledgment in response to a request containing a block size option
> according to RFC2348.
> 
> The format of an OACK response is described in RFC2347 as
> 
> +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+
> |  opc  |  opt1  | 0 | value1 | 0 |  optN  | 0 | valueN | 0 |
> +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+
> 
> The current implementation of find_option() only works if
> 
> * blksize is the first option
> * lwip_strnstr() ignores the length parameter,
>   i.e. is implemented via strstr()
> 
> The OACK messages starts with  0x00 0x06. If 'blksize' is the first option,
> strstr() reports a match when the first parameter points to 0x06. Adding
> the string length of 'blksize' plus 2 to the location of the 0x06 byte
> points to the value.
> 
> Find_option() would report a match for option 'blksize' if the response
> contained an option called 'foo_blksize_bar'. In this case find_option()
> would return 'bar' as the value string.
> 
> If 'blksize' were the second option, find_option() would return a pointer
> to the second character of the value string.
> 
> Furthermore find_option() does not detect if the value string is NUL
> terminated. This may lead to a buffer overrun.
> 
> Provide an implementation that correctly steps from option to option.
> 
> Fixes: 27d7ccda94fa ("net: lwip: tftp: add support of blksize option to client")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
> v2:
> 	new patch
> ---

That's indeed much better! Thanks for the fix. One minor comment below.
In any case:
Reviewed-by: Jerome Forissier <jerome.forissier at linaro.org>
Tested-by: Jerome Forissier <jerome.forissier at linaro.org> (qemu_arm64_lwip)

>  lib/lwip/lwip/src/apps/tftp/tftp.c | 54 +++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/lwip/lwip/src/apps/tftp/tftp.c b/lib/lwip/lwip/src/apps/tftp/tftp.c
> index 56aeabc4d73..e85e3623066 100644
> --- a/lib/lwip/lwip/src/apps/tftp/tftp.c
> +++ b/lib/lwip/lwip/src/apps/tftp/tftp.c
> @@ -264,19 +264,55 @@ static u16_t payload_size(void)
>    return TFTP_DEFAULT_BLOCK_SIZE;
>  }
>  
> +/**
> + * find_option() - check if OACK message contains option
> + *
> + * @p:		message buffer
> + * @option:	option key
> + * Return:	option value
> + */
>  static const char *
>  find_option(struct pbuf *p, const char *option)
>  {
> -  int i;
> -  u16_t optlen = strlen(option);
> -  const char *b = p->payload;
> -
> -  for (i = 0; i + optlen + 1 < p->len; i++) {
> -    if (lwip_strnstr(b + i, option, optlen))
> -      return b + i + optlen + 2;
> -  }
> +	const char *pos = p->payload;
> +	int rem = p->len;
> +
> +	/*
> +	 * According to RFC 2347 the OACK packet has the following format:
> +	 *
> +	 * +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+
> +	 * |  opc  |  opt1  | 0 | value1 | 0 |  optN  | 0 | valueN | 0 |
> +	 * +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+
> +	 */
> +
> +	/* Skip opc */
> +	pos += 2;
> +	rem -= 2;
> +	if (rem <= 0)
> +		return NULL;
> +
> +	for (;;) {
> +		int len;
> +		int match;
> +
> +		len = strnlen(pos, rem) + 1;
> +		if (rem < len)
> +			break;
> +		match = strcmp(pos, option);
> +		/* Skip option */
> +		pos += len;
> +		rem -= len;
> +		len = strnlen(pos, rem) + 1;
> +		if (rem < len)
> +			break;
> +		if (!match)
> +			return pos;

This reads somewhat counter-intuitively ("if no match"). How about
renaming 'match' to 'cmp' or 'diff'?

> +		/* Skip value */
> +		pos += len;
> +		rem -= len;
> +	}
>  
> -  return NULL;
> +	return NULL;
>  }
>  
>  static void

Regards,
-- 
Jerome