diff mbox series

[v5,3/4] net-lwip: wget: add LMB and buffer checks

Message ID 20250417132718.2023555-4-jerome.forissier@linaro.org
State New
Headers show
Series NET_LWIP LMB fixes | expand

Commit Message

Jerome Forissier April 17, 2025, 1:26 p.m. UTC
Legacy NET wget invokes a store_block() function which performs buffer
validation (LMB, address wrapping). Do the same with NET_LWIP.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Acked-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

(no changes since v4)

Changes in v4:
- The 'silent' boolean in stored in struct wget_http_info (same as NET)

Changes in v3:
- store_block(): add Sphinx-like documentation
- store_block(): do not print to the console if ctx->silent

Changes in v2:
- httpc_recv_cb(): add a call to altcp_abort(). Otherwise the transfer
continues and we try to write later blocks which makes no sense if one
has been rejected already. Thanks Sughosh G. for testing and reporting.

 net/lwip/wget.c | 64 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 55 insertions(+), 9 deletions(-)

Comments

Sughosh Ganu April 24, 2025, 12:51 p.m. UTC | #1
On Thu, 17 Apr 2025 at 18:57, Jerome Forissier
<jerome.forissier@linaro.org> wrote:
>
> Legacy NET wget invokes a store_block() function which performs buffer
> validation (LMB, address wrapping). Do the same with NET_LWIP.
>
> Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
> Suggested-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Acked-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---

Tested that the LMB checks are performed and wget/tftp commands do not
overwrite on an existing LMB reservation.

Tested-by: Sughosh Ganu <sughosh.ganu@linaro.org>

-sughosh

>
> (no changes since v4)
>
> Changes in v4:
> - The 'silent' boolean in stored in struct wget_http_info (same as NET)
>
> Changes in v3:
> - store_block(): add Sphinx-like documentation
> - store_block(): do not print to the console if ctx->silent
>
> Changes in v2:
> - httpc_recv_cb(): add a call to altcp_abort(). Otherwise the transfer
> continues and we try to write later blocks which makes no sense if one
> has been rejected already. Thanks Sughosh G. for testing and reporting.
>
>  net/lwip/wget.c | 64 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 55 insertions(+), 9 deletions(-)
>
> diff --git a/net/lwip/wget.c b/net/lwip/wget.c
> index 63583e4c6e7..4ec00de96b2 100644
> --- a/net/lwip/wget.c
> +++ b/net/lwip/wget.c
> @@ -6,6 +6,7 @@
>  #include <display_options.h>
>  #include <efi_loader.h>
>  #include <image.h>
> +#include <linux/kconfig.h>
>  #include <lwip/apps/http_client.h>
>  #include "lwip/altcp_tls.h"
>  #include <lwip/errno.h>
> @@ -202,11 +203,58 @@ static int parse_legacy_arg(char *arg, char *nurl, size_t rem)
>         return 0;
>  }
>
> +/**
> + * store_block() - copy received data
> + *
> + * This function is called by the receive callback to copy a block of data
> + * into its final location (ctx->daddr). Before doing so, it checks if the copy
> + * is allowed.
> + *
> + * @ctx: the context for the current transfer
> + * @src: the data received from the TCP stack
> + * @len: the length of the data
> + */
> +static int store_block(struct wget_ctx *ctx, void *src, u16_t len)
> +{
> +       ulong store_addr = ctx->daddr;
> +       uchar *ptr;
> +
> +       /* Avoid overflow */
> +       if (wget_info->buffer_size && wget_info->buffer_size < ctx->size + len)
> +               return -1;
> +
> +       if (CONFIG_IS_ENABLED(LMB) && wget_info->set_bootdev) {
> +               if (store_addr + len < store_addr ||
> +                   lmb_read_check(store_addr, len)) {
> +                       if (!wget_info->silent) {
> +                               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);
> +
> +       ctx->daddr += len;
> +       ctx->size += len;
> +       if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
> +               if (!wget_info->silent)
> +                       printf("#");
> +               ctx->prevsize = ctx->size;
> +       }
> +
> +       return 0;
> +}
> +
>  static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
>                            err_t err)
>  {
>         struct wget_ctx *ctx = arg;
>         struct pbuf *buf;
> +       err_t ret;
>
>         if (!pbuf)
>                 return ERR_BUF;
> @@ -215,19 +263,17 @@ static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
>                 ctx->start_time = get_timer(0);
>
>         for (buf = pbuf; buf; buf = buf->next) {
> -               memcpy((void *)ctx->daddr, buf->payload, buf->len);
> -               ctx->daddr += buf->len;
> -               ctx->size += buf->len;
> -               if (!wget_info->silent &&
> -                   ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
> -                       printf("#");
> -                       ctx->prevsize = ctx->size;
> +               if (store_block(ctx, buf->payload, buf->len) < 0) {
> +                       altcp_abort(pcb);
> +                       ret = ERR_BUF;
> +                       goto out;
>                 }
>         }
> -
>         altcp_recved(pcb, pbuf->tot_len);
> +       ret = ERR_OK;
> +out:
>         pbuf_free(pbuf);
> -       return ERR_OK;
> +       return ret;
>  }
>
>  static void httpc_result_cb(void *arg, httpc_result_t httpc_result,
> --
> 2.43.0
>
diff mbox series

Patch

diff --git a/net/lwip/wget.c b/net/lwip/wget.c
index 63583e4c6e7..4ec00de96b2 100644
--- a/net/lwip/wget.c
+++ b/net/lwip/wget.c
@@ -6,6 +6,7 @@ 
 #include <display_options.h>
 #include <efi_loader.h>
 #include <image.h>
+#include <linux/kconfig.h>
 #include <lwip/apps/http_client.h>
 #include "lwip/altcp_tls.h"
 #include <lwip/errno.h>
@@ -202,11 +203,58 @@  static int parse_legacy_arg(char *arg, char *nurl, size_t rem)
 	return 0;
 }
 
+/**
+ * store_block() - copy received data
+ *
+ * This function is called by the receive callback to copy a block of data
+ * into its final location (ctx->daddr). Before doing so, it checks if the copy
+ * is allowed.
+ *
+ * @ctx: the context for the current transfer
+ * @src: the data received from the TCP stack
+ * @len: the length of the data
+ */
+static int store_block(struct wget_ctx *ctx, void *src, u16_t len)
+{
+	ulong store_addr = ctx->daddr;
+	uchar *ptr;
+
+	/* Avoid overflow */
+	if (wget_info->buffer_size && wget_info->buffer_size < ctx->size + len)
+		return -1;
+
+	if (CONFIG_IS_ENABLED(LMB) && wget_info->set_bootdev) {
+		if (store_addr + len < store_addr ||
+		    lmb_read_check(store_addr, len)) {
+			if (!wget_info->silent) {
+				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);
+
+	ctx->daddr += len;
+	ctx->size += len;
+	if (ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
+		if (!wget_info->silent)
+			printf("#");
+		ctx->prevsize = ctx->size;
+	}
+
+	return 0;
+}
+
 static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
 			   err_t err)
 {
 	struct wget_ctx *ctx = arg;
 	struct pbuf *buf;
+	err_t ret;
 
 	if (!pbuf)
 		return ERR_BUF;
@@ -215,19 +263,17 @@  static err_t httpc_recv_cb(void *arg, struct altcp_pcb *pcb, struct pbuf *pbuf,
 		ctx->start_time = get_timer(0);
 
 	for (buf = pbuf; buf; buf = buf->next) {
-		memcpy((void *)ctx->daddr, buf->payload, buf->len);
-		ctx->daddr += buf->len;
-		ctx->size += buf->len;
-		if (!wget_info->silent &&
-		    ctx->size - ctx->prevsize > PROGRESS_PRINT_STEP_BYTES) {
-			printf("#");
-			ctx->prevsize = ctx->size;
+		if (store_block(ctx, buf->payload, buf->len) < 0) {
+			altcp_abort(pcb);
+			ret = ERR_BUF;
+			goto out;
 		}
 	}
-
 	altcp_recved(pcb, pbuf->tot_len);
+	ret = ERR_OK;
+out:
 	pbuf_free(pbuf);
-	return ERR_OK;
+	return ret;
 }
 
 static void httpc_result_cb(void *arg, httpc_result_t httpc_result,