diff mbox series

[v2,1/6] net: wget: prevent overwriting reserved memory

Message ID 20230901102542.609239-2-masahisa.kojima@linaro.org
State New
Headers show
Series Add EFI HTTP boot support | expand

Commit Message

Masahisa Kojima Sept. 1, 2023, 10:25 a.m. UTC 
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>
---
 net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

Comments

Simon Glass Sept. 2, 2023, 12:09 a.m. UTC | #1 
Hi Masahisa,

On Fri, 1 Sept 2023 at 04:26, Masahisa Kojima
<masahisa.kojima@linaro.org> 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>
> ---
>  net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/net/wget.c b/net/wget.c
> index 2dbfeb1a1d..8bf4db4d04 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,32 @@ 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 */
>
> +#ifdef CONFIG_LMB
> +static ulong wget_load_size;
> +#endif
> +
> +/**
> + * wget_init_max_size() - initialize maximum load size
> + *
> + * Return:     0 if success, -1 if fails
> + */
> +static int wget_init_load_size(void)
> +{
> +#ifdef CONFIG_LMB

please can you use if IS_ENABLED() throughout?

> +       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;
> +#endif
> +       return 0;
> +}
> +
>  /**
>   * store_block() - store block in memory
>   * @src: source of data
> @@ -63,10 +93,24 @@ 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);
> +#ifdef 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) {
> +               puts("\nwget error: ");
> +               puts("trying to overwrite reserved memory...\n");
> +               return -1;
> +       }
> +#endif
> +       ptr = map_sysmem(store_addr, len);
>         memcpy(ptr, src, len);
>         unmap_sysmem(ptr);
>
> @@ -240,25 +284,37 @@ 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);
> +                                       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);
> +                                       return;
> +                               }
>                         }
>                 }
>         }
> @@ -420,6 +476,12 @@ void wget_start(void)
>         debug_cond(DEBUG_WGET,
>                    "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
>
> +       if (wget_init_load_size()) {
> +               puts("\nwget error: ");
> +               puts("trying to overwrite reserved memory...\n");

I think it is OK to use printf() here.

> +               return;

Needs to return the error code

> +       }
> +
>         net_set_timeout_handler(wget_timeout, wget_timeout_handler);
>         tcp_set_tcp_handler(wget_handler);
>
> --
> 2.34.1
>

Regards,
Simon
Masahisa Kojima Sept. 5, 2023, 7:13 a.m. UTC | #2 
Hi Simon,

On Sat, 2 Sept 2023 at 09:09, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Fri, 1 Sept 2023 at 04:26, Masahisa Kojima
> <masahisa.kojima@linaro.org> 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>
> > ---
> >  net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 69 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/wget.c b/net/wget.c
> > index 2dbfeb1a1d..8bf4db4d04 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,32 @@ 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 */
> >
> > +#ifdef CONFIG_LMB
> > +static ulong wget_load_size;
> > +#endif
> > +
> > +/**
> > + * wget_init_max_size() - initialize maximum load size
> > + *
> > + * Return:     0 if success, -1 if fails
> > + */
> > +static int wget_init_load_size(void)
> > +{
> > +#ifdef CONFIG_LMB
>
> please can you use if IS_ENABLED() throughout?
OK.

>
> > +       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;
> > +#endif
> > +       return 0;
> > +}
> > +
> >  /**
> >   * store_block() - store block in memory
> >   * @src: source of data
> > @@ -63,10 +93,24 @@ 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);
> > +#ifdef 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) {
> > +               puts("\nwget error: ");
> > +               puts("trying to overwrite reserved memory...\n");
> > +               return -1;
> > +       }
> > +#endif
> > +       ptr = map_sysmem(store_addr, len);
> >         memcpy(ptr, src, len);
> >         unmap_sysmem(ptr);
> >
> > @@ -240,25 +284,37 @@ 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);
> > +                                       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);
> > +                                       return;
> > +                               }
> >                         }
> >                 }
> >         }
> > @@ -420,6 +476,12 @@ void wget_start(void)
> >         debug_cond(DEBUG_WGET,
> >                    "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
> >
> > +       if (wget_init_load_size()) {
> > +               puts("\nwget error: ");
> > +               puts("trying to overwrite reserved memory...\n");
>
> I think it is OK to use printf() here.
OK.

>
> > +               return;
>
> Needs to return the error code

I will modify it to notify the caller of the  error by calling
"net_set_state(NETLOOP_FAIL);"
instead of returning the error code.

Thanks,
Masahisa Kojima

>
> > +       }
> > +
> >         net_set_timeout_handler(wget_timeout, wget_timeout_handler);
> >         tcp_set_tcp_handler(wget_handler);
> >
> > --
> > 2.34.1
> >
>
> Regards,
> Simon
diff mbox series

Patch

diff --git a/net/wget.c b/net/wget.c
index 2dbfeb1a1d..8bf4db4d04 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,32 @@  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 */
 
+#ifdef CONFIG_LMB
+static ulong wget_load_size;
+#endif
+
+/**
+ * wget_init_max_size() - initialize maximum load size
+ *
+ * Return:	0 if success, -1 if fails
+ */
+static int wget_init_load_size(void)
+{
+#ifdef CONFIG_LMB
+	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;
+#endif
+	return 0;
+}
+
 /**
  * store_block() - store block in memory
  * @src: source of data
@@ -63,10 +93,24 @@  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);
+#ifdef 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) {
+		puts("\nwget error: ");
+		puts("trying to overwrite reserved memory...\n");
+		return -1;
+	}
+#endif
+	ptr = map_sysmem(store_addr, len);
 	memcpy(ptr, src, len);
 	unmap_sysmem(ptr);
 
@@ -240,25 +284,37 @@  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);
+					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);
+					return;
+				}
 			}
 		}
 	}
@@ -420,6 +476,12 @@  void wget_start(void)
 	debug_cond(DEBUG_WGET,
 		   "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
 
+	if (wget_init_load_size()) {
+		puts("\nwget error: ");
+		puts("trying to overwrite reserved memory...\n");
+		return;
+	}
+
 	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
 	tcp_set_tcp_handler(wget_handler);