[PATCH v4] net: tftp: Add client support for RFC 7440

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

Add support for RFC 7440: "TFTP Windowsize Option".

This optional feature allows the client and server
to negotiate a window size of consecutive blocks to send as an
alternative for replacing the single-block lockstep schema.

windowsize can be defined statically during compilation by
setting CONFIG_TFTP_WINDOWSIZE, or defined in runtime by
setting an environment variable: "tftpwindowsize"
If not defined, the windowsize is set to 1, meaning that it
behaves as it was never defined.

Choosing the appropriate windowsize depends on the specific
network topology, underlying NIC.
You should test various windowsize scenarios and see which
best work for you.

Setting a windowsize too big can actually decreases performance.

Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
Reviewed-by: Marek Vasut <marex@denx.de>
---
v2:
 * Don't send windowsize option on tftpput, as it's not implemented yet.
 * Don't send NACK for every out of order block that arrives, one nack
   is enough.
v3:
 * Add option CONFIG_TFTP_WINDOWSIZE to kconfig with default 1.
 * Fixed some spelling errors.
 * Took assignment out of a loop.
 * simplified variable increment.
v4:
 * send ack for last packet, so the server can finish
   the tranfer gracefully and not in timeout.

 README      |  5 ++++
 net/Kconfig |  9 ++++++
 net/tftp.c  | 81 +++++++++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 87 insertions(+), 8 deletions(-)

diff --git a/README b/README
index be9e6391d6..686474a2f1 100644
--- a/README
+++ b/README
@@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete):
 		  downloads succeed with high packet loss rates, or with
 		  unreliable TFTP servers or client hardware.
 
+  tftpwindowsize	- if this is set, the value is used for TFTP's
+		  window size as described by RFC 7440.
+		  This means the count of blocks we can receive before
+		  sending ack to server.
+
   vlan		- When set to a value < 4095 the traffic over
 		  Ethernet is encapsulated/received over 802.1q
 		  VLAN tagged frames.
diff --git a/net/Kconfig b/net/Kconfig
index ac6d0cf8a6..7916ae305f 100644
--- a/net/Kconfig
+++ b/net/Kconfig
@@ -49,4 +49,13 @@ config TFTP_BLOCKSIZE
 	  almost-MTU block sizes.
 	  You can also activate CONFIG_IP_DEFRAG to set a larger block.
 
+config TFTP_WINDOWSIZE
+	int "TFTP window size"
+	default 1
+	help
+	  Default TFTP window size.
+	  RFC7440 defines an optional window size of transmits,
+	  before an ack response is required.
+	  The default TFTP implementation implies a window size of 1.
+
 endif   # if NET
diff --git a/net/tftp.c b/net/tftp.c
index be24e63075..72d23e1574 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -5,7 +5,6 @@
  * Copyright 2011 Comelit Group SpA,
  *                Luca Ceresoli <luca.ceresoli@comelit.it>
  */
-
 #include <common.h>
 #include <command.h>
 #include <efi_loader.h>
@@ -95,6 +94,12 @@ static int	tftp_tsize;
 /* The number of hashes we printed */
 static short	tftp_tsize_num_hash;
 #endif
+/* The window size negotiated */
+static ushort	tftp_windowsize;
+/* Next block to send ack to */
+static ushort	tftp_next_ack;
+/* Last nack block we send */
+static ushort	tftp_last_nack;
 #ifdef CONFIG_CMD_TFTPPUT
 /* 1 if writing, else 0 */
 static int	tftp_put_active;
@@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN];
  * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file)
  */
 
+/* When windowsize is defined to 1,
+ * tftp behaves the same way as it was
+ * never declared
+ */
+#ifdef CONFIG_TFTP_WINDOWSIZE
+#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE
+#else
+#define TFTP_WINDOWSIZE 1
+#endif
+
 static unsigned short tftp_block_size = TFTP_BLOCK_SIZE;
 static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
+static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE;
 
 static inline int store_block(int block, uchar *src, unsigned int len)
 {
@@ -348,6 +364,14 @@ static void tftp_send(void)
 		/* try for more effic. blk size */
 		pkt += sprintf((char *)pkt, "blksize%c%d%c",
 				0, tftp_block_size_option, 0);
+
+		/* try for more effic. window size.
+		 * Implemented only for tftp get.
+		 * Don't bother sending if it's 1
+		 */
+		if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1)
+			pkt += sprintf((char *)pkt, "windowsize%c%d%c",
+					0, tftp_window_size_option, 0);
 		len = pkt - xp;
 		break;
 
@@ -500,7 +524,18 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 				      (char *)pkt + i + 6, tftp_tsize);
 			}
 #endif
+			if (strcmp((char *)pkt + i,  "windowsize") == 0) {
+				tftp_windowsize =
+					simple_strtoul((char *)pkt + i + 11,
+						       NULL, 10);
+			    debug("windowsize = %s, %d\n",
+				  (char *)pkt + i + 11, tftp_windowsize);
+			}
+
 		}
+
+		tftp_next_ack = tftp_windowsize;
+
 #ifdef CONFIG_CMD_TFTPPUT
 		if (tftp_put_active) {
 			/* Get ready to send the first block */
@@ -514,12 +549,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		if (len < 2)
 			return;
 		len -= 2;
-		tftp_cur_block = ntohs(*(__be16 *)pkt);
+
+		if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
+			debug("Received unexpected block: %d, expected: %d\n",
+			      ntohs(*(__be16 *)pkt),
+			      (ushort)(tftp_cur_block + 1));
+			/*
+			 * If one packet is dropped most likely
+			 * all other buffers in the window
+			 * that will arrive will cause a sending NACK.
+			 * This just overwellms the server, let's just send one.
+			 */
+			if (tftp_last_nack != tftp_cur_block) {
+				tftp_send();
+				tftp_last_nack = tftp_cur_block;
+				tftp_next_ack = (ushort)(tftp_cur_block +
+							 tftp_windowsize);
+			}
+			break;
+		}
+
+		tftp_cur_block++;
 
 		update_block_number();
 
 		if (tftp_state == STATE_SEND_RRQ)
-			debug("Server did not acknowledge timeout option!\n");
+			debug("Server did not acknowledge the options!\n");
 
 		if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK ||
 		    tftp_state == STATE_RECV_WRQ) {
@@ -557,10 +612,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		 *	Acknowledge the block just received, which will prompt
 		 *	the remote for the next one.
 		 */
-		tftp_send();
+		if (tftp_cur_block == tftp_next_ack) {
+			tftp_send();
+			tftp_next_ack += tftp_windowsize;
+		}
 
-		if (len < tftp_block_size)
+		if (len < tftp_block_size) {
+			tftp_send();
 			tftp_complete();
+		}
 		break;
 
 	case TFTP_ERROR:
@@ -634,6 +694,10 @@ void tftp_start(enum proto_t protocol)
 	if (ep != NULL)
 		tftp_block_size_option = simple_strtol(ep, NULL, 10);
 
+	ep = env_get("tftpwindowsize");
+	if (ep != NULL)
+		tftp_window_size_option = simple_strtol(ep, NULL, 10);
+
 	ep = env_get("tftptimeout");
 	if (ep != NULL)
 		timeout_ms = simple_strtol(ep, NULL, 10);
@@ -655,8 +719,8 @@ void tftp_start(enum proto_t protocol)
 	}
 #endif
 
-	debug("TFTP blocksize = %i, timeout = %ld ms\n",
-	      tftp_block_size_option, timeout_ms);
+	debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
+	      tftp_block_size_option, tftp_window_size_option, timeout_ms);
 
 	tftp_remote_ip = net_server_ip;
 	if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) {
@@ -752,7 +816,8 @@ void tftp_start(enum proto_t protocol)
 		tftp_our_port = simple_strtol(ep, NULL, 10);
 #endif
 	tftp_cur_block = 0;
-
+	tftp_windowsize = 1;
+	tftp_last_nack = 0;
 	/* zero out server ether in case the server ip has changed */
 	memset(net_server_ethaddr, 0, 6);
 	/* Revert tftp_block_size to dflt */
-- 
2.26.2

[PATCH v4] net: tftp: Add client support for RFC 7440

[rahasij]

Ramon Fried-4 wrote
> +			if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> +				tftp_windowsize =
> +					simple_strtoul((char *)pkt + i + 11,
> +						       NULL, 10);
> +			    debug("windowsize = %s, %d\n",
> +				  (char *)pkt + i + 11, tftp_windowsize);
> +			}
> +
>  		}
> -- 
> 2.26.2

As per RFC2347, the option string is case insensitive. I fixed this for
other options in following patch

https://lists.denx.de/pipermail/u-boot/2020-May/412472.html

Please use strcasecmp() instead of strcmp().

As per RFC7440, the value received from server should be less than or equal
to the value proposed by client . This check should be added here, and error
packet must be generated in case of failure. 

Above patch implements ERR pkt generation and should be applied first.




--
Sent from: http://u-boot.10912.n7.nabble.com/

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

Thanks.

On Fri, May 22, 2020 at 3:29 AM rahasij <rahasij@linux.microsoft.com> wrote:
>
> Ramon Fried-4 wrote
> > +                     if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > +                             tftp_windowsize =
> > +                                     simple_strtoul((char *)pkt + i + 11,
> > +                                                    NULL, 10);
> > +                         debug("windowsize = %s, %d\n",
> > +                               (char *)pkt + i + 11, tftp_windowsize);
> > +                     }
> > +
> >               }
> > --
> > 2.26.2
>
> As per RFC2347, the option string is case insensitive. I fixed this for
> other options in following patch
>
> https://lists.denx.de/pipermail/u-boot/2020-May/412472.html
>
> Please use strcasecmp() instead of strcmp().
>
> As per RFC7440, the value received from server should be less than or equal
> to the value proposed by client . This check should be added here, and error
> packet must be generated in case of failure.
>
> Above patch implements ERR pkt generation and should be applied first.
>
>
>
>
> --
> Sent from: http://u-boot.10912.n7.nabble.com/

[PATCH v4] net: tftp: Add client support for RFC 7440

[Matthias Brugger]

On 19/05/2020 21:25, Ramon Fried wrote:
> Add support for RFC 7440: "TFTP Windowsize Option".
> 
> This optional feature allows the client and server
> to negotiate a window size of consecutive blocks to send as an
> alternative for replacing the single-block lockstep schema.
> 
> windowsize can be defined statically during compilation by
> setting CONFIG_TFTP_WINDOWSIZE, or defined in runtime by
> setting an environment variable: "tftpwindowsize"
> If not defined, the windowsize is set to 1, meaning that it
> behaves as it was never defined.
> 
> Choosing the appropriate windowsize depends on the specific
> network topology, underlying NIC.
> You should test various windowsize scenarios and see which
> best work for you.
> 
> Setting a windowsize too big can actually decreases performance.
> 
> Signed-off-by: Ramon Fried <rfried.dev@gmail.com>
> Reviewed-by: Marek Vasut <marex@denx.de>
> ---
> v2:
>  * Don't send windowsize option on tftpput, as it's not implemented yet.
>  * Don't send NACK for every out of order block that arrives, one nack
>    is enough.
> v3:
>  * Add option CONFIG_TFTP_WINDOWSIZE to kconfig with default 1.
>  * Fixed some spelling errors.
>  * Took assignment out of a loop.
>  * simplified variable increment.
> v4:
>  * send ack for last packet, so the server can finish
>    the tranfer gracefully and not in timeout.
> 
>  README      |  5 ++++
>  net/Kconfig |  9 ++++++
>  net/tftp.c  | 81 +++++++++++++++++++++++++++++++++++++++++++++++------
>  3 files changed, 87 insertions(+), 8 deletions(-)
> 
> diff --git a/README b/README
> index be9e6391d6..686474a2f1 100644
> --- a/README
> +++ b/README
> @@ -3522,6 +3522,11 @@ List of environment variables (most likely not complete):
>  		  downloads succeed with high packet loss rates, or with
>  		  unreliable TFTP servers or client hardware.
>  
> +  tftpwindowsize	- if this is set, the value is used for TFTP's
> +		  window size as described by RFC 7440.
> +		  This means the count of blocks we can receive before
> +		  sending ack to server.
> +
>    vlan		- When set to a value < 4095 the traffic over
>  		  Ethernet is encapsulated/received over 802.1q
>  		  VLAN tagged frames.
> diff --git a/net/Kconfig b/net/Kconfig
> index ac6d0cf8a6..7916ae305f 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -49,4 +49,13 @@ config TFTP_BLOCKSIZE
>  	  almost-MTU block sizes.
>  	  You can also activate CONFIG_IP_DEFRAG to set a larger block.
>  
> +config TFTP_WINDOWSIZE
> +	int "TFTP window size"
> +	default 1
> +	help
> +	  Default TFTP window size.
> +	  RFC7440 defines an optional window size of transmits,
> +	  before an ack response is required.
> +	  The default TFTP implementation implies a window size of 1.
> +
>  endif   # if NET
> diff --git a/net/tftp.c b/net/tftp.c
> index be24e63075..72d23e1574 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -5,7 +5,6 @@
>   * Copyright 2011 Comelit Group SpA,
>   *                Luca Ceresoli <luca.ceresoli@comelit.it>
>   */
> -
>  #include <common.h>
>  #include <command.h>
>  #include <efi_loader.h>
> @@ -95,6 +94,12 @@ static int	tftp_tsize;
>  /* The number of hashes we printed */
>  static short	tftp_tsize_num_hash;
>  #endif
> +/* The window size negotiated */
> +static ushort	tftp_windowsize;
> +/* Next block to send ack to */
> +static ushort	tftp_next_ack;
> +/* Last nack block we send */
> +static ushort	tftp_last_nack;
>  #ifdef CONFIG_CMD_TFTPPUT
>  /* 1 if writing, else 0 */
>  static int	tftp_put_active;
> @@ -134,8 +139,19 @@ static char tftp_filename[MAX_LEN];
>   * (but those using CONFIG_IP_DEFRAG may want to set a larger block in cfg file)
>   */
>  
> +/* When windowsize is defined to 1,
> + * tftp behaves the same way as it was
> + * never declared
> + */
> +#ifdef CONFIG_TFTP_WINDOWSIZE
> +#define TFTP_WINDOWSIZE CONFIG_TFTP_WINDOWSIZE
> +#else
> +#define TFTP_WINDOWSIZE 1
> +#endif
> +
>  static unsigned short tftp_block_size = TFTP_BLOCK_SIZE;
>  static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
> +static unsigned short tftp_window_size_option = TFTP_WINDOWSIZE;
>  
>  static inline int store_block(int block, uchar *src, unsigned int len)
>  {
> @@ -348,6 +364,14 @@ static void tftp_send(void)
>  		/* try for more effic. blk size */
>  		pkt += sprintf((char *)pkt, "blksize%c%d%c",
>  				0, tftp_block_size_option, 0);
> +
> +		/* try for more effic. window size.
> +		 * Implemented only for tftp get.
> +		 * Don't bother sending if it's 1
> +		 */
> +		if (tftp_state == STATE_SEND_RRQ && tftp_window_size_option > 1)

I think it makes more sense to check:
if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ)

Because I understand that the tftp_state will change while the
tftp_window_size_option is set or at compile time or through the environment. So
we can save the check of the tftp_state if we have the default value.

Regards,
Matthias

> +			pkt += sprintf((char *)pkt, "windowsize%c%d%c",
> +					0, tftp_window_size_option, 0);
>  		len = pkt - xp;
>  		break;
>  
> @@ -500,7 +524,18 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  				      (char *)pkt + i + 6, tftp_tsize);
>  			}
>  #endif
> +			if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> +				tftp_windowsize =
> +					simple_strtoul((char *)pkt + i + 11,
> +						       NULL, 10);
> +			    debug("windowsize = %s, %d\n",
> +				  (char *)pkt + i + 11, tftp_windowsize);
> +			}
> +
>  		}
> +
> +		tftp_next_ack = tftp_windowsize;
> +
>  #ifdef CONFIG_CMD_TFTPPUT
>  		if (tftp_put_active) {
>  			/* Get ready to send the first block */
> @@ -514,12 +549,32 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  		if (len < 2)
>  			return;
>  		len -= 2;
> -		tftp_cur_block = ntohs(*(__be16 *)pkt);
> +
> +		if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> +			debug("Received unexpected block: %d, expected: %d\n",
> +			      ntohs(*(__be16 *)pkt),
> +			      (ushort)(tftp_cur_block + 1));
> +			/*
> +			 * If one packet is dropped most likely
> +			 * all other buffers in the window
> +			 * that will arrive will cause a sending NACK.
> +			 * This just overwellms the server, let's just send one.
> +			 */
> +			if (tftp_last_nack != tftp_cur_block) {
> +				tftp_send();
> +				tftp_last_nack = tftp_cur_block;
> +				tftp_next_ack = (ushort)(tftp_cur_block +
> +							 tftp_windowsize);
> +			}
> +			break;
> +		}
> +
> +		tftp_cur_block++;
>  
>  		update_block_number();
>  
>  		if (tftp_state == STATE_SEND_RRQ)
> -			debug("Server did not acknowledge timeout option!\n");
> +			debug("Server did not acknowledge the options!\n");
>  
>  		if (tftp_state == STATE_SEND_RRQ || tftp_state == STATE_OACK ||
>  		    tftp_state == STATE_RECV_WRQ) {
> @@ -557,10 +612,15 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>  		 *	Acknowledge the block just received, which will prompt
>  		 *	the remote for the next one.
>  		 */
> -		tftp_send();
> +		if (tftp_cur_block == tftp_next_ack) {
> +			tftp_send();
> +			tftp_next_ack += tftp_windowsize;
> +		}
>  
> -		if (len < tftp_block_size)
> +		if (len < tftp_block_size) {
> +			tftp_send();
>  			tftp_complete();
> +		}
>  		break;
>  
>  	case TFTP_ERROR:
> @@ -634,6 +694,10 @@ void tftp_start(enum proto_t protocol)
>  	if (ep != NULL)
>  		tftp_block_size_option = simple_strtol(ep, NULL, 10);
>  
> +	ep = env_get("tftpwindowsize");
> +	if (ep != NULL)
> +		tftp_window_size_option = simple_strtol(ep, NULL, 10);
> +
>  	ep = env_get("tftptimeout");
>  	if (ep != NULL)
>  		timeout_ms = simple_strtol(ep, NULL, 10);
> @@ -655,8 +719,8 @@ void tftp_start(enum proto_t protocol)
>  	}
>  #endif
>  
> -	debug("TFTP blocksize = %i, timeout = %ld ms\n",
> -	      tftp_block_size_option, timeout_ms);
> +	debug("TFTP blocksize = %i, TFTP windowsize = %d timeout = %ld ms\n",
> +	      tftp_block_size_option, tftp_window_size_option, timeout_ms);
>  
>  	tftp_remote_ip = net_server_ip;
>  	if (!net_parse_bootfile(&tftp_remote_ip, tftp_filename, MAX_LEN)) {
> @@ -752,7 +816,8 @@ void tftp_start(enum proto_t protocol)
>  		tftp_our_port = simple_strtol(ep, NULL, 10);
>  #endif
>  	tftp_cur_block = 0;
> -
> +	tftp_windowsize = 1;
> +	tftp_last_nack = 0;
>  	/* zero out server ether in case the server ip has changed */
>  	memset(net_server_ethaddr, 0, 6);
>  	/* Revert tftp_block_size to dflt */
> 

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

On Sat, May 23, 2020 at 8:40 PM Matthias Brugger <matthias.bgg@gmail.com> wrote:
...
>
> I think it makes more sense to check:
> if (tftp_window_size_option > 1 && tftp_state == STATE_SEND_RRQ)
>
> Because I understand that the tftp_state will change while the
> tftp_window_size_option is set or at compile time or through the environment. So
> we can save the check of the tftp_state if we have the default value.
>
> Regards,
> Matthias
>
The tftp_state can be only STATE_SEND_RRQ or STATE_SEND_WRQ and it's checked
one time per transfer, it won't change in the middle of transfer.
This code is run one time per transfer.
Thanks,
Ramon

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ravik Hasija]

Ramon Fried-4 wrote
> +			if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> For servers that doesnt support windowsize option the above check could
> result in accessing memory outside of valid range. Please check if (i+11)
> < len before comparing the strings.
> 
> 
> +
> +		if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> +			debug("Received unexpected block: %d, expected: %d\n",
> +			      ntohs(*(__be16 *)pkt),
> +			      (ushort)(tftp_cur_block + 1));
> +			/*
> +			 * If one packet is dropped most likely
> +			 * all other buffers in the window
> +			 * that will arrive will cause a sending NACK.
> +			 * This just overwellms the server, let's just send one.
> +			 */
> +			if (tftp_last_nack != tftp_cur_block) {
> +				tftp_send();
> +				tftp_last_nack = tftp_cur_block;
> +				tftp_next_ack = (ushort)(tftp_cur_block +
> +							 tftp_windowsize);
> +			}
> +			break;
> +		}
> +
> +		tftp_cur_block++;
> 
> Monotonically increasing the tftp_cur_block will cause error for cases
> where sequence number wraps around as tftp_cur_block is ulong, thus during
> wraparound the check ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)
> will fail and incorrectly generate ACK, and the connection will eventually
> be terminated once the retry is exhausted. Please modulo the increment
> with TFTP_SEQUENCE_SIZE.  
> -- 
> 2.26.2

Quoted from: 
http://u-boot.10912.n7.nabble.com/PATCH-v4-net-tftp-Add-client-support-for-RFC-7440-tp412754.html




--
Sent from: http://u-boot.10912.n7.nabble.com/

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ramon Fried]

On Wed, Jun 3, 2020 at 5:55 AM Ravik Hasija <rahasij@linux.microsoft.com> wrote:
>
> Ramon Fried-4 wrote
> > +                     if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > For servers that doesnt support windowsize option the above check could
> > result in accessing memory outside of valid range. Please check if (i+11)
> > < len before comparing the strings.
This is the same handling as all other possible configurations,
following the same code.
I agree that this needs reworking, but I'll do it in a different patch
all together.

> >
> >
> > +
> > +             if (ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)) {
> > +                     debug("Received unexpected block: %d, expected: %d\n",
> > +                           ntohs(*(__be16 *)pkt),
> > +                           (ushort)(tftp_cur_block + 1));
> > +                     /*
> > +                      * If one packet is dropped most likely
> > +                      * all other buffers in the window
> > +                      * that will arrive will cause a sending NACK.
> > +                      * This just overwellms the server, let's just send one.
> > +                      */
> > +                     if (tftp_last_nack != tftp_cur_block) {
> > +                             tftp_send();
> > +                             tftp_last_nack = tftp_cur_block;
> > +                             tftp_next_ack = (ushort)(tftp_cur_block +
> > +                                                      tftp_windowsize);
> > +                     }
> > +                     break;
> > +             }
> > +
> > +             tftp_cur_block++;
> >
> > Monotonically increasing the tftp_cur_block will cause error for cases
> > where sequence number wraps around as tftp_cur_block is ulong, thus during
> > wraparound the check ntohs(*(__be16 *)pkt) != (ushort)(tftp_cur_block + 1)
> > will fail and incorrectly generate ACK, and the connection will eventually
> > be terminated once the retry is exhausted. Please modulo the increment
> > with TFTP_SEQUENCE_SIZE.
True, will fix.
Thanks.
> > --
> > 2.26.2
>
> Quoted from:
> http://u-boot.10912.n7.nabble.com/PATCH-v4-net-tftp-Add-client-support-for-RFC-7440-tp412754.html
>
>
>
>
> --
> Sent from: http://u-boot.10912.n7.nabble.com/

[PATCH v4] net: tftp: Add client support for RFC 7440

[Ravik Hasija]

On Wed, Jun 3, 2020 at 5:55 AM Ravik Hasija <rahasij at .microsoft>
wrote:
>
> Ramon Fried-4 wrote
> > +                     if (strcmp((char *)pkt + i,  "windowsize") == 0) {
> > For servers that doesnt support windowsize option the above check could
> > result in accessing memory outside of valid range. Please check if
> (i+11)
> > < len before comparing the strings.
> This is the same handling as all other possible configurations,
> following the same code.
> I agree that this needs reworking, but I'll do it in a different patch
> all together.

Yes, the other options need to be fixed as well. However, we should fix
(i+11)<len in this patch itself, and restructure others, and windowsize (if
needed) in a different patch, since the tftpd (commonly used for TFTP
server) does not support windowsize option while it supports other options
(tsize,blksize,timeout etc.), and there is a high chance that the client
code might crash in that case.

&lt;/quote>




--
Sent from: http://u-boot.10912.n7.nabble.com/