[PATCH master] RISC-V: boot: uncompress: determine piggy data bounds before relocation

[PATCH master] RISC-V: boot: uncompress: determine piggy data bounds before relocation

[Ahmad Fatoum]

Like on ARM, the PBL common code does not relocate the piggy data and
instead keeps pointers into the old image's location.

For relocate_to_current_adr, this doesn't matter, because both
destination and source address are the same, but when running from ROM
or flash relocate_to_adr will only copy up to __bss_start, so
determining the piggy data bounds _must_ happen before relocation.

We did so, but input_data_len, which references the image end was
executed after relocation unearthing two bugs at once:

  - The compiler cached input_data_end from before relocation, so
    the runtime offset wasn't being added

  - Even with runtime offset added, we were pointing at the new
    address, but the old piggy data wasn't copied there (On erizo,
    it's still in ROM)

Fix these by evaluating the bounds before relocation and add a comment,
why it needs to be there. The same comment will also be added at the
code's origin in the ARM architecture support.

Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
Please apply to master.
---
 arch/riscv/boot/uncompress.c      | 6 +++---
 arch/riscv/include/asm/sections.h | 4 +++-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/boot/uncompress.c b/arch/riscv/boot/uncompress.c
index 411cefb0e31b..b4e010998a4a 100644
--- a/arch/riscv/boot/uncompress.c
+++ b/arch/riscv/boot/uncompress.c
@@ -32,8 +32,11 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
 	void *pg_start, *pg_end;
 	unsigned long pc = get_pc();
 
+	/* piggy data is not relocated, so determine the bounds now */
 	pg_start = input_data + get_runtime_offset();
 	pg_end = input_data_end + get_runtime_offset();
+	pg_len = pg_end - pg_start;
+	uncompressed_len = input_data_len();
 
 	/*
 	 * If we run from inside the memory just relocate the binary
@@ -45,9 +48,6 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
 	else
 		relocate_to_adr(membase);
 
-	pg_len = pg_end - pg_start;
-	uncompressed_len = input_data_len();
-
 	barebox_base = riscv_mem_barebox_image(membase, endmem,
 					       uncompressed_len + MAX_BSS_SIZE);
 
diff --git a/arch/riscv/include/asm/sections.h b/arch/riscv/include/asm/sections.h
index 725fd8db474e..6673648bcd58 100644
--- a/arch/riscv/include/asm/sections.h
+++ b/arch/riscv/include/asm/sections.h
@@ -15,9 +15,11 @@ extern char __dynsym_end[];
 extern char input_data[];
 extern char input_data_end[];
 
+unsigned long get_runtime_offset(void);
+
 static inline unsigned int input_data_len(void)
 {
-	return get_unaligned((const u32 *)(input_data_end - 4));
+	return get_unaligned((const u32 *)(input_data_end + get_runtime_offset() - 4));
 }
 
 #endif
-- 
2.30.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] RISC-V: boot: uncompress: determine piggy data bounds before relocation

[Antony Pavlov]

On Sat,  3 Apr 2021 09:03:32 +0200
Ahmad Fatoum <ahmad@a3f.at> wrote:

Hi Ahmad!

I have just tested this patch on erizo FPGA. Barebox work fine.

-- 
Best regards,
  Antony Pavlov

> Like on ARM, the PBL common code does not relocate the piggy data and
> instead keeps pointers into the old image's location.
> 
> For relocate_to_current_adr, this doesn't matter, because both
> destination and source address are the same, but when running from ROM
> or flash relocate_to_adr will only copy up to __bss_start, so
> determining the piggy data bounds _must_ happen before relocation.
> 
> We did so, but input_data_len, which references the image end was
> executed after relocation unearthing two bugs at once:
> 
>   - The compiler cached input_data_end from before relocation, so
>     the runtime offset wasn't being added
> 
>   - Even with runtime offset added, we were pointing at the new
>     address, but the old piggy data wasn't copied there (On erizo,
>     it's still in ROM)
> 
> Fix these by evaluating the bounds before relocation and add a comment,
> why it needs to be there. The same comment will also be added at the
> code's origin in the ARM architecture support.
> 
> Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
> Please apply to master.
> ---
>  arch/riscv/boot/uncompress.c      | 6 +++---
>  arch/riscv/include/asm/sections.h | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/boot/uncompress.c b/arch/riscv/boot/uncompress.c
> index 411cefb0e31b..b4e010998a4a 100644
> --- a/arch/riscv/boot/uncompress.c
> +++ b/arch/riscv/boot/uncompress.c
> @@ -32,8 +32,11 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>  	void *pg_start, *pg_end;
>  	unsigned long pc = get_pc();
>  
> +	/* piggy data is not relocated, so determine the bounds now */
>  	pg_start = input_data + get_runtime_offset();
>  	pg_end = input_data_end + get_runtime_offset();
> +	pg_len = pg_end - pg_start;
> +	uncompressed_len = input_data_len();
>  
>  	/*
>  	 * If we run from inside the memory just relocate the binary
> @@ -45,9 +48,6 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>  	else
>  		relocate_to_adr(membase);
>  
> -	pg_len = pg_end - pg_start;
> -	uncompressed_len = input_data_len();
> -
>  	barebox_base = riscv_mem_barebox_image(membase, endmem,
>  					       uncompressed_len + MAX_BSS_SIZE);
>  
> diff --git a/arch/riscv/include/asm/sections.h b/arch/riscv/include/asm/sections.h
> index 725fd8db474e..6673648bcd58 100644
> --- a/arch/riscv/include/asm/sections.h
> +++ b/arch/riscv/include/asm/sections.h
> @@ -15,9 +15,11 @@ extern char __dynsym_end[];
>  extern char input_data[];
>  extern char input_data_end[];
>  
> +unsigned long get_runtime_offset(void);
> +
>  static inline unsigned int input_data_len(void)
>  {
> -	return get_unaligned((const u32 *)(input_data_end - 4));
> +	return get_unaligned((const u32 *)(input_data_end + get_runtime_offset() - 4));
>  }
>  
>  #endif
> -- 
> 2.30.0
> 



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH master] RISC-V: boot: uncompress: determine piggy data bounds before relocation

[Sascha Hauer]

On Sat, Apr 03, 2021 at 09:03:32AM +0200, Ahmad Fatoum wrote:
> Like on ARM, the PBL common code does not relocate the piggy data and
> instead keeps pointers into the old image's location.
> 
> For relocate_to_current_adr, this doesn't matter, because both
> destination and source address are the same, but when running from ROM
> or flash relocate_to_adr will only copy up to __bss_start, so
> determining the piggy data bounds _must_ happen before relocation.
> 
> We did so, but input_data_len, which references the image end was
> executed after relocation unearthing two bugs at once:
> 
>   - The compiler cached input_data_end from before relocation, so
>     the runtime offset wasn't being added
> 
>   - Even with runtime offset added, we were pointing at the new
>     address, but the old piggy data wasn't copied there (On erizo,
>     it's still in ROM)
> 
> Fix these by evaluating the bounds before relocation and add a comment,
> why it needs to be there. The same comment will also be added at the
> code's origin in the ARM architecture support.
> 
> Reported-by: Antony Pavlov <antonynpavlov@gmail.com>
> Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
> ---
> Please apply to master.

Did that, thanks

Sascha

> ---
>  arch/riscv/boot/uncompress.c      | 6 +++---
>  arch/riscv/include/asm/sections.h | 4 +++-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/riscv/boot/uncompress.c b/arch/riscv/boot/uncompress.c
> index 411cefb0e31b..b4e010998a4a 100644
> --- a/arch/riscv/boot/uncompress.c
> +++ b/arch/riscv/boot/uncompress.c
> @@ -32,8 +32,11 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>  	void *pg_start, *pg_end;
>  	unsigned long pc = get_pc();
>  
> +	/* piggy data is not relocated, so determine the bounds now */
>  	pg_start = input_data + get_runtime_offset();
>  	pg_end = input_data_end + get_runtime_offset();
> +	pg_len = pg_end - pg_start;
> +	uncompressed_len = input_data_len();
>  
>  	/*
>  	 * If we run from inside the memory just relocate the binary
> @@ -45,9 +48,6 @@ void __noreturn barebox_pbl_start(unsigned long membase, unsigned long memsize,
>  	else
>  		relocate_to_adr(membase);
>  
> -	pg_len = pg_end - pg_start;
> -	uncompressed_len = input_data_len();
> -
>  	barebox_base = riscv_mem_barebox_image(membase, endmem,
>  					       uncompressed_len + MAX_BSS_SIZE);
>  
> diff --git a/arch/riscv/include/asm/sections.h b/arch/riscv/include/asm/sections.h
> index 725fd8db474e..6673648bcd58 100644
> --- a/arch/riscv/include/asm/sections.h
> +++ b/arch/riscv/include/asm/sections.h
> @@ -15,9 +15,11 @@ extern char __dynsym_end[];
>  extern char input_data[];
>  extern char input_data_end[];
>  
> +unsigned long get_runtime_offset(void);
> +
>  static inline unsigned int input_data_len(void)
>  {
> -	return get_unaligned((const u32 *)(input_data_end - 4));
> +	return get_unaligned((const u32 *)(input_data_end + get_runtime_offset() - 4));
>  }
>  
>  #endif
> -- 
> 2.30.0
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox