Re: [PATCH v4 1/8] net: wget: prevent overwriting reserved memory

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Fri, Sep 22, 2023 at 04:11:12PM +0900, Masahisa Kojima wrote:
> This introduces the valid range check to store the received
> blocks using lmb. The same logic is implemented in tftp.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  net/wget.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/net/wget.c b/net/wget.c
> index 2dbfeb1a1d..a48a8cb624 100644
> --- a/net/wget.c
> +++ b/net/wget.c
> @@ -4,16 +4,20 @@
>   * Copyright Duncan Hare <dh@synoia.com> 2017
>   */
>
> +#include <asm/global_data.h>
>  #include <command.h>
>  #include <common.h>
>  #include <display_options.h>
>  #include <env.h>
>  #include <image.h>
> +#include <lmb.h>
>  #include <mapmem.h>
>  #include <net.h>
>  #include <net/tcp.h>
>  #include <net/wget.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  static const char bootfile1[] = "GET ";
>  static const char bootfile3[] = " HTTP/1.0\r\n\r\n";
>  static const char http_eom[] = "\r\n\r\n";
> @@ -55,6 +59,29 @@ static unsigned int retry_tcp_ack_num;	/* TCP retry acknowledge number*/
>  static unsigned int retry_tcp_seq_num;	/* TCP retry sequence number */
>  static int retry_len;			/* TCP retry length */
>
> +static ulong wget_load_size;
> +
> +/**
> + * wget_init_max_size() - initialize maximum load size
> + *
> + * Return:	0 if success, -1 if fails
> + */
> +static int wget_init_load_size(void)
> +{
> +	struct lmb lmb;
> +	phys_size_t max_size;
> +
> +	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> +
> +	max_size = lmb_get_free_size(&lmb, image_load_addr);
> +	if (!max_size)
> +		return -1;
> +
> +	wget_load_size = max_size;
> +
> +	return 0;
> +}
> +
>  /**
>   * store_block() - store block in memory
>   * @src: source of data
> @@ -63,10 +90,25 @@ static int retry_len;			/* TCP retry length */
>   */
>  static inline int store_block(uchar *src, unsigned int offset, unsigned int len)
>  {
> +	ulong store_addr = image_load_addr + offset;
>  	ulong newsize = offset + len;
>  	uchar *ptr;
>
> -	ptr = map_sysmem(image_load_addr + offset, len);
> +	if (IS_ENABLED(CONFIG_LMB)) {
> +		ulong end_addr = image_load_addr + wget_load_size;
> +
> +		if (!end_addr)
> +			end_addr = ULONG_MAX;
> +
> +		if (store_addr < image_load_addr ||
> +		    store_addr + len > end_addr) {
> +			printf("\nwget error: ");
> +			printf("trying to overwrite reserved memory...\n");
> +			return -1;
> +		}
> +	}
> +
> +	ptr = map_sysmem(store_addr, len);
>  	memcpy(ptr, src, len);
>  	unmap_sysmem(ptr);
>
> @@ -240,25 +282,39 @@ static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
>
>  			net_boot_file_size = 0;
>
> -			if (len > hlen)
> -				store_block(pkt + hlen, 0, len - hlen);
> +			if (len > hlen) {
> +				if (store_block(pkt + hlen, 0, len - hlen) != 0) {
> +					wget_loop_state = NETLOOP_FAIL;
> +					wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> +					net_set_state(NETLOOP_FAIL);
> +					return;
> +				}
> +			}
>
>  			debug_cond(DEBUG_WGET,
>  				   "wget: Connected Pkt %p hlen %x\n",
>  				   pkt, hlen);
>
>  			for (i = 0; i < pkt_q_idx; i++) {
> +				int err;
> +
>  				ptr1 = map_sysmem(
>  					(phys_addr_t)(pkt_q[i].pkt),
>  					pkt_q[i].len);
> -				store_block(ptr1,
> -					    pkt_q[i].tcp_seq_num -
> -					    initial_data_seq_num,
> -					    pkt_q[i].len);
> +				err = store_block(ptr1,
> +					  pkt_q[i].tcp_seq_num -
> +					  initial_data_seq_num,
> +					  pkt_q[i].len);
>  				unmap_sysmem(ptr1);
>  				debug_cond(DEBUG_WGET,
>  					   "wget: Connctd pkt Q %p len %x\n",
>  					   pkt_q[i].pkt, pkt_q[i].len);
> +				if (err) {
> +					wget_loop_state = NETLOOP_FAIL;
> +					wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> +					net_set_state(NETLOOP_FAIL);
> +					return;
> +				}
>  			}
>  		}
>  	}
> @@ -330,6 +386,7 @@ static void wget_handler(uchar *pkt, u16 dport,
>  				len) != 0) {
>  			wget_fail("wget: store error\n",
>  				  tcp_seq_num, tcp_ack_num, action);
> +			net_set_state(NETLOOP_FAIL);
>  			return;
>  		}
>
> @@ -420,6 +477,15 @@ void wget_start(void)
>  	debug_cond(DEBUG_WGET,
>  		   "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
>
> +	if (IS_ENABLED(CONFIG_LMB)) {
> +		if (wget_init_load_size()) {
> +			printf("\nwget error: ");
> +			printf("trying to overwrite reserved memory...\n");
> +			net_set_state(NETLOOP_FAIL);
> +			return;
> +		}
> +	}
> +
>  	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
>  	tcp_set_tcp_handler(wget_handler);
>
> --
> 2.34.1
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>