[U-Boot] [PATCH v2 08/10] bootstage: Correct relocation algorithm

[Heinrich Schuchardt]

On 10/22/19 1:26 AM, Simon Glass wrote:
> At present bootstage relocation assumes that it is possible to point back
> to memory available before relocation, so it does not relocate the
> strings. However this is not the case on some platforms, such as x86 which
> uses the cache as RAM and loses access to this when the cache is enabled.
>
> Move the relocation step to before U-Boot relocates, expand the allocated
> region to include space for the strings and relocate the strings at the
> same time as the bootstage records.
>
> This ensures that bootstage data can remain accessible from TPL through
> SPL to U-Boot before/after relocation.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>

Hello Simon,

this merged patch seems to be incorrect. I compiled sandbox_defconfig
with clang and ran it with valgrind.

We allocate memory in bootstage_init() for gd->bootstage. But from
bootstage_get_size() we return a size that is larger than what we have
allocated and use that larger memory area in reloc_bootstage(). See
output below.

sudo docker pull trini/u-boot-gitlab-ci-runner:bionic-20200112-17Jan2020
sudo docker run -it 2e501a804876 /bin/bash
cd ~
git clone https://gitlab.denx.de/u-boot/u-boot.git
cd u-boot
sudo apt-get update
sudo apt-get install clang valgrind
make CC=clang HOSTCC=clang sandbox_defconfig
make CC=clang HOSTCC=clang -j8
valgrind ./u-boot -d arch/sandbox/dts/test.dtb

==89423== Memcheck, a memory error detector
==89423== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==89423== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==89423== Command: ./u-boot -d arch/sandbox/dts/test.dtb
==89423==


U-Boot 2020.01-00812-gc98c2b07e5 (Jan 25 2020 - 07:36:12 +0000)

Model: sandbox
DRAM:  128 MiB
==89423== Invalid read of size 8
==89423==    at 0x4D4E90: memcpy (string.c:543)
==89423==    by 0x424F88: reloc_bootstage (board_f.c:702)
==89423==    by 0x424B22: initcall_run_list (initcall.h:40)
==89423==    by 0x424B22: board_init_f (board_f.c:1019)
==89423==    by 0x402755: main (start.c:386)
==89423==  Address 0xa66f4e8 is 0 bytes after a block of size 968 alloc'd
==89423==    at 0x4C2FB0F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==89423==    by 0x42E740: bootstage_init (bootstage.c:522)
==89423==    by 0x424B84: initf_bootstage (board_f.c:806)
==89423==    by 0x424B22: initcall_run_list (initcall.h:40)
==89423==    by 0x424B22: board_init_f (board_f.c:1019)
==89423==    by 0x402755: main (start.c:386)
==89423==
==89423== Invalid read of size 8
==89423==    at 0x4D4EA4: memcpy (string.c:542)
==89423==    by 0x424F88: reloc_bootstage (board_f.c:702)
==89423==    by 0x424B22: initcall_run_list (initcall.h:40)
==89423==    by 0x424B22: board_init_f (board_f.c:1019)
==89423==    by 0x402755: main (start.c:386)
==89423==  Address 0xa66f4f0 is 8 bytes after a block of size 968 alloc'd
==89423==    at 0x4C2FB0F: malloc (in
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==89423==    by 0x42E740: bootstage_init (bootstage.c:522)
==89423==    by 0x424B84: initf_bootstage (board_f.c:806)
==89423==    by 0x424B22: initcall_run_list (initcall.h:40)
==89423==    by 0x424B22: board_init_f (board_f.c:1019)
==89423==    by 0x402755: main (start.c:386)
==89423==
WDT:   Started with servicing (60s timeout)
MMC:   mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD)
In:    serial
Out:   vidconsole
Err:   vidconsole
Model: sandbox
SCSI:
Net:   eth0: eth at 10002000, eth5: eth at 10003000, eth3: sbe5, eth1:
eth at 10004000
Hit any key to stop autoboot:  0
=>

Best regards

Heinrich

> ---
>
> Changes in v2: None
>
>   common/board_f.c   |  1 +
>   common/board_r.c   |  1 -
>   common/bootstage.c | 25 ++++++++++++++++++++++---
>   3 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/common/board_f.c b/common/board_f.c
> index 4852a3b0d84..e3591cbaebd 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -696,6 +696,7 @@ static int reloc_bootstage(void)
>   		      gd->bootstage, gd->new_bootstage, size);
>   		memcpy(gd->new_bootstage, gd->bootstage, size);
>   		gd->bootstage = gd->new_bootstage;
> +		bootstage_relocate();
>   	}
>   #endif
>
> diff --git a/common/board_r.c b/common/board_r.c
> index d6fb5047a26..c1ecb06b743 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -670,7 +670,6 @@ static init_fnc_t init_sequence_r[] = {
>   #ifdef CONFIG_SYS_NONCACHED_MEMORY
>   	initr_noncached,
>   #endif
> -	bootstage_relocate,
>   #ifdef CONFIG_OF_LIVE
>   	initr_of_live,
>   #endif
> diff --git a/common/bootstage.c b/common/bootstage.c
> index 4557ed4508c..e8b7bbf81a6 100644
> --- a/common/bootstage.c
> +++ b/common/bootstage.c
> @@ -53,14 +53,23 @@ int bootstage_relocate(void)
>   {
>   	struct bootstage_data *data = gd->bootstage;
>   	int i;
> +	char *ptr;
> +
> +	/* Figure out where to relocate the strings to */
> +	ptr = (char *)(data + 1);
>
>   	/*
>   	 * Duplicate all strings.  They may point to an old location in the
>   	 * program .text section that can eventually get trashed.
>   	 */
>   	debug("Relocating %d records\n", data->rec_count);
> -	for (i = 0; i < data->rec_count; i++)
> -		data->record[i].name = strdup(data->record[i].name);
> +	for (i = 0; i < data->rec_count; i++) {
> +		const char *from = data->record[i].name;
> +
> +		strcpy(ptr, from);
> +		data->record[i].name = ptr;
> +		ptr += strlen(ptr) + 1;
> +	}
>
>   	return 0;
>   }
> @@ -490,7 +499,17 @@ int bootstage_unstash(const void *base, int size)
>
>   int bootstage_get_size(void)
>   {
> -	return sizeof(struct bootstage_data);
> +	struct bootstage_data *data = gd->bootstage;
> +	struct bootstage_record *rec;
> +	int size;
> +	int i;
> +
> +	size = sizeof(struct bootstage_data);
> +	for (rec = data->record, i = 0; i < data->rec_count;
> +	     i++, rec++)
> +		size += strlen(rec->name) + 1;
> +
> +	return size;
>   }
>
>   int bootstage_init(bool first)
>