[PATCH 0/3] net: tftp: Fixes for tftp rollover

[PATCH 0/3] net: tftp: Fixes for tftp rollover

[Ley Foon Tan]

This patch series fix the tftp block number rollover bugs
when sending and receiving large file (block number
greater than 16-bit).

Tested receiving and sending large file with block
number greater than 0xffff, verified content
is 100% matched. Tested file size 128MB and 256MB.

Ley Foon Tan (3):
  net: tftp: Fix tftp_prev_block counter update
  net: tftp: Fix store_block offset calculation
  net: tftp: Fix load_block offset calculation

 net/tftp.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

-- 
2.19.0

[PATCH 1/3] net: tftp: Fix tftp_prev_block counter update

[Ley Foon Tan]

Fixes missing update to tftp_prev_block counter before increase
tftp_cur_block counter when do the tftpput operation.

tftp_prev_block counter is used in update_block_number() function to
check whether block number (sequence number) is rollover. This bug
cause the tftpput command fail to upload a large file when block
number is greater than 16-bit (0xFFFF).

Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
---
 net/tftp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/tftp.c b/net/tftp.c
index c05b7b5532b9..9ca7db256112 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -478,6 +478,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 				int block = ntohs(*s);
 				int ack_ok = (tftp_cur_block == block);
 
+				tftp_prev_block = tftp_cur_block;
 				tftp_cur_block = (unsigned short)(block + 1);
 				update_block_number();
 				if (ack_ok)
-- 
2.19.0

[PATCH 2/3] net: tftp: Fix store_block offset calculation

[Ley Foon Tan]

tftp_cur_block start with 1 for first block, but tftp_cur_block counter is
start with zero when block number is rollover. The existing code
"tftp_cur_block - 1" will cause the block number become -1 in store_block()
when tftp_cur_block is 0 when tftp_cur_block is rollover.

The fix pass in tftp_cur_block to store_block() and minus the
tftp_block_size when do the offset calculation.

Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
---
 net/tftp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/tftp.c b/net/tftp.c
index 9ca7db256112..6e68a427d4cf 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -143,7 +143,8 @@ static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
 
 static inline int store_block(int block, uchar *src, unsigned int len)
 {
-	ulong offset = block * tftp_block_size + tftp_block_wrap_offset;
+	ulong offset = block * tftp_block_size + tftp_block_wrap_offset -
+			tftp_block_size;
 	ulong newsize = offset + len;
 	ulong store_addr = tftp_load_addr + offset;
 #ifdef CONFIG_SYS_DIRECT_FLASH_TFTP
@@ -597,7 +598,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
 		timeout_count_max = tftp_timeout_count_max;
 		net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
 
-		if (store_block(tftp_cur_block - 1, pkt + 2, len)) {
+		if (store_block(tftp_cur_block, pkt + 2, len)) {
 			eth_halt();
 			net_set_state(NETLOOP_FAIL);
 			break;
-- 
2.19.0

[PATCH 3/3] net: tftp: Fix load_block offset calculation

[Ley Foon Tan]

When load the last block, the "len" might not be a block size. This cause
loading the incorrect last block data.

The fix change "len" to tftp_block_size and minus one tftp_block_size
for offset calculation.

Use same offset calculation formula as in store_block().

Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
---
 net/tftp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tftp.c b/net/tftp.c
index 6e68a427d4cf..292e7b4cddcf 100644
--- a/net/tftp.c
+++ b/net/tftp.c
@@ -218,7 +218,8 @@ static void new_transfer(void)
 static int load_block(unsigned block, uchar *dst, unsigned len)
 {
 	/* We may want to get the final block from the previous set */
-	ulong offset = ((int)block - 1) * len + tftp_block_wrap_offset;
+	ulong offset = block * tftp_block_size + tftp_block_wrap_offset -
+		       tftp_block_size;
 	ulong tosend = len;
 
 	tosend = min(net_boot_file_size - offset, tosend);
-- 
2.19.0

[PATCH 0/3] net: tftp: Fixes for tftp rollover

[Tan, Ley Foon]

> -----Original Message-----
> From: Tan, Ley Foon <ley.foon.tan@intel.com>
> Sent: Tuesday, August 25, 2020 10:27 AM
> To: u-boot at lists.denx.de
> Cc: Joe Hershberger <joe.hershberger@ni.com>; Ley Foon Tan
> <lftan.linux@gmail.com>; See, Chin Liang <chin.liang.see@intel.com>; Tan,
> Ley Foon <ley.foon.tan@intel.com>
> Subject: [PATCH 0/3] net: tftp: Fixes for tftp rollover
> 
> This patch series fix the tftp block number rollover bugs when sending and
> receiving large file (block number greater than 16-bit).
> 
> Tested receiving and sending large file with block number greater than 0xffff,
> verified content is 100% matched. Tested file size 128MB and 256MB.
> 
> Ley Foon Tan (3):
>   net: tftp: Fix tftp_prev_block counter update
>   net: tftp: Fix store_block offset calculation
>   net: tftp: Fix load_block offset calculation
> 
>  net/tftp.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Hi Joe

Any comment for these 3 patches?

Thanks.

Regards
Ley Foon

[PATCH 1/3] net: tftp: Fix tftp_prev_block counter update

[Ramon Fried]

On Tue, Aug 25, 2020 at 5:27 AM Ley Foon Tan <ley.foon.tan@intel.com> wrote:
>
> Fixes missing update to tftp_prev_block counter before increase
> tftp_cur_block counter when do the tftpput operation.
>
> tftp_prev_block counter is used in update_block_number() function to
> check whether block number (sequence number) is rollover. This bug
> cause the tftpput command fail to upload a large file when block
> number is greater than 16-bit (0xFFFF).
>
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> ---
>  net/tftp.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index c05b7b5532b9..9ca7db256112 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -478,6 +478,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>                                 int block = ntohs(*s);
>                                 int ack_ok = (tftp_cur_block == block);
>
> +                               tftp_prev_block = tftp_cur_block;
>                                 tftp_cur_block = (unsigned short)(block + 1);
>                                 update_block_number();
>                                 if (ack_ok)
> --
> 2.19.0
>
Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

[PATCH 2/3] net: tftp: Fix store_block offset calculation

[Ramon Fried]

On Tue, Aug 25, 2020 at 5:27 AM Ley Foon Tan <ley.foon.tan@intel.com> wrote:
>
> tftp_cur_block start with 1 for first block, but tftp_cur_block counter is
> start with zero when block number is rollover. The existing code
> "tftp_cur_block - 1" will cause the block number become -1 in store_block()
> when tftp_cur_block is 0 when tftp_cur_block is rollover.
>
> The fix pass in tftp_cur_block to store_block() and minus the
> tftp_block_size when do the offset calculation.
>
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> ---
>  net/tftp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index 9ca7db256112..6e68a427d4cf 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -143,7 +143,8 @@ static unsigned short tftp_block_size_option = CONFIG_TFTP_BLOCKSIZE;
>
>  static inline int store_block(int block, uchar *src, unsigned int len)
>  {
> -       ulong offset = block * tftp_block_size + tftp_block_wrap_offset;
> +       ulong offset = block * tftp_block_size + tftp_block_wrap_offset -
> +                       tftp_block_size;
>         ulong newsize = offset + len;
>         ulong store_addr = tftp_load_addr + offset;
>  #ifdef CONFIG_SYS_DIRECT_FLASH_TFTP
> @@ -597,7 +598,7 @@ static void tftp_handler(uchar *pkt, unsigned dest, struct in_addr sip,
>                 timeout_count_max = tftp_timeout_count_max;
>                 net_set_timeout_handler(timeout_ms, tftp_timeout_handler);
>
> -               if (store_block(tftp_cur_block - 1, pkt + 2, len)) {
> +               if (store_block(tftp_cur_block, pkt + 2, len)) {
>                         eth_halt();
>                         net_set_state(NETLOOP_FAIL);
>                         break;
> --
> 2.19.0
>
Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

[PATCH 3/3] net: tftp: Fix load_block offset calculation

[Ramon Fried]

On Tue, Aug 25, 2020 at 5:27 AM Ley Foon Tan <ley.foon.tan@intel.com> wrote:
>
> When load the last block, the "len" might not be a block size. This cause
> loading the incorrect last block data.
>
> The fix change "len" to tftp_block_size and minus one tftp_block_size
> for offset calculation.
>
> Use same offset calculation formula as in store_block().
>
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> ---
>  net/tftp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/tftp.c b/net/tftp.c
> index 6e68a427d4cf..292e7b4cddcf 100644
> --- a/net/tftp.c
> +++ b/net/tftp.c
> @@ -218,7 +218,8 @@ static void new_transfer(void)
>  static int load_block(unsigned block, uchar *dst, unsigned len)
>  {
>         /* We may want to get the final block from the previous set */
> -       ulong offset = ((int)block - 1) * len + tftp_block_wrap_offset;
> +       ulong offset = block * tftp_block_size + tftp_block_wrap_offset -
> +                      tftp_block_size;
>         ulong tosend = len;
>
>         tosend = min(net_boot_file_size - offset, tosend);
> --
> 2.19.0
>
Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

[PATCH 1/3] net: tftp: Fix tftp_prev_block counter update

[Tom Rini]

On Tue, Aug 25, 2020 at 10:26:35AM +0800, Ley Foon Tan wrote:

> Fixes missing update to tftp_prev_block counter before increase
> tftp_cur_block counter when do the tftpput operation.
> 
> tftp_prev_block counter is used in update_block_number() function to
> check whether block number (sequence number) is rollover. This bug
> cause the tftpput command fail to upload a large file when block
> number is greater than 16-bit (0xFFFF).
> 
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201001/cf2da63f/attachment.sig>

[PATCH 2/3] net: tftp: Fix store_block offset calculation

[Tom Rini]

On Tue, Aug 25, 2020 at 10:26:36AM +0800, Ley Foon Tan wrote:

> tftp_cur_block start with 1 for first block, but tftp_cur_block counter is
> start with zero when block number is rollover. The existing code
> "tftp_cur_block - 1" will cause the block number become -1 in store_block()
> when tftp_cur_block is 0 when tftp_cur_block is rollover.
> 
> The fix pass in tftp_cur_block to store_block() and minus the
> tftp_block_size when do the offset calculation.
> 
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201001/d1f3d5dc/attachment.sig>

[PATCH 3/3] net: tftp: Fix load_block offset calculation

[Tom Rini]

On Tue, Aug 25, 2020 at 10:26:37AM +0800, Ley Foon Tan wrote:

> When load the last block, the "len" might not be a block size. This cause
> loading the incorrect last block data.
> 
> The fix change "len" to tftp_block_size and minus one tftp_block_size
> for offset calculation.
> 
> Use same offset calculation formula as in store_block().
> 
> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com>
> Reviewed-By: Ramon Fried <rfried.dev@gmail.com>

Applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201001/6f656cb1/attachment.sig>