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.foriss...@linaro.org>
>> Suggested-by: Sughosh Ganu <sughosh.g...@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,
--
Jerome
>
>> + 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,
>
