| Message ID | 20250409122007.2560668-2-jerome.forissier@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | NET_LWIP LMB fixes | expand |
On 09.04.25 14:20, Jerome Forissier 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> > --- > net/lwip/wget.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 41 insertions(+), 8 deletions(-) > > diff --git a/net/lwip/wget.c b/net/lwip/wget.c > index 14f27d42998..746c8164d66 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/timeouts.h> > @@ -201,11 +202,44 @@ static int parse_legacy_arg(char *arg, char *nurl, size_t rem) > return 0; > } > Please, add Sphinx style descriptions to all functions. > +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)) { > + printf("\nwget error: "); > + printf("trying to overwrite reserved memory...\n"); Output while an EFI application is consuming the HTTP protocol should be avoided as it is not compatible with the EFI console. If you want console output in the wget command, please, implement it there and not in a library function called by efi_net_do_request(). > + 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) { > + printf("#"); ditto Best regards Heinrich > + 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; > @@ -214,18 +248,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 (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,
On 4/9/25 16:17, Heinrich Schuchardt wrote: > On 09.04.25 14:20, Jerome Forissier 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> >> --- >> net/lwip/wget.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 41 insertions(+), 8 deletions(-) >> >> diff --git a/net/lwip/wget.c b/net/lwip/wget.c >> index 14f27d42998..746c8164d66 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/timeouts.h> >> @@ -201,11 +202,44 @@ static int parse_legacy_arg(char *arg, char *nurl, size_t rem) >> return 0; >> } >> > > Please, add Sphinx style descriptions to all functions. OK >> +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)) { >> + printf("\nwget error: "); >> + printf("trying to overwrite reserved memory...\n"); > > Output while an EFI application is consuming the HTTP protocol should be > avoided as it is not compatible with the EFI console. What does printf() do in this case (EFI application)? If it's just ignored then I suppose it should be OK to have the printf() right where the error occurs, no? > If you want console output in the wget command, please, implement it > there and not in a library function called by efi_net_do_request(). That will make the code slightly more complicated, although not much (some additional status in struct wget_ctx). Regards,
diff --git a/net/lwip/wget.c b/net/lwip/wget.c index 14f27d42998..746c8164d66 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/timeouts.h> @@ -201,11 +202,44 @@ static int parse_legacy_arg(char *arg, char *nurl, size_t rem) return 0; } +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)) { + 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) { + 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; @@ -214,18 +248,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 (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,
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> --- net/lwip/wget.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 8 deletions(-)