[PATCH] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction

[PATCH] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction

[Thaison Phan]

Hi,

The aarch64 bootm image handler for barebox can choose a load address
that is not 4KB aligned. This can result in unexpected behavior with
the ADRP instruction that is available in 64 bit ARM architectures.
ADRP forms a PC-relative address to a 4KB page where the bottom 12
bits of the current PC will be masked out. When the load address of
the barebox image is not 4KB aligned ADRP can end up forming an
address that starts from an invalid page of memory or the wrong page
of memory that was expected to be formed. The following patch aligns
the load address for the next barebox image to be 4KB aligned to
accommodate the ADRP instruction.

Thanks,
Thaison

---
 arch/arm/lib64/armlinux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/lib64/armlinux.c b/arch/arm/lib64/armlinux.c
index 8382ffdf1b..d66963dae5 100644
--- a/arch/arm/lib64/armlinux.c
+++ b/arch/arm/lib64/armlinux.c
@@ -43,6 +43,8 @@ static struct image_handler aarch64_fit_handler = {
        .filetype = filetype_oftree,
 };

+#define ADRP_PAGE_MASK 0x1000
+
 static int do_bootm_barebox(struct image_data *data)
 {
        void (*fn)(unsigned long x0, unsigned long x1, unsigned long x2,
@@ -55,7 +57,7 @@ static int do_bootm_barebox(struct image_data *data)
        if (ret)
                goto out;

-       barebox = start;
+       barebox = ALIGN(start, ADRP_PAGE_MASK);

        ret = bootm_load_os(data, barebox);
        if (ret)
--
2.25.1

Re: [PATCH] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction

[Sascha Hauer]

Hi Thaison,

On Tue, Feb 06, 2024 at 09:37:47PM -0500, Thaison Phan wrote:
> Hi,
> 
> The aarch64 bootm image handler for barebox can choose a load address
> that is not 4KB aligned. This can result in unexpected behavior with
> the ADRP instruction that is available in 64 bit ARM architectures.
> ADRP forms a PC-relative address to a 4KB page where the bottom 12
> bits of the current PC will be masked out. When the load address of
> the barebox image is not 4KB aligned ADRP can end up forming an
> address that starts from an invalid page of memory or the wrong page
> of memory that was expected to be formed. The following patch aligns
> the load address for the next barebox image to be 4KB aligned to
> accommodate the ADRP instruction.
> 
> Thanks,
> Thaison
> 
> ---
>  arch/arm/lib64/armlinux.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/lib64/armlinux.c b/arch/arm/lib64/armlinux.c
> index 8382ffdf1b..d66963dae5 100644
> --- a/arch/arm/lib64/armlinux.c
> +++ b/arch/arm/lib64/armlinux.c
> @@ -43,6 +43,8 @@ static struct image_handler aarch64_fit_handler = {
>         .filetype = filetype_oftree,
>  };
> 
> +#define ADRP_PAGE_MASK 0x1000
> +
>  static int do_bootm_barebox(struct image_data *data)
>  {
>         void (*fn)(unsigned long x0, unsigned long x1, unsigned long x2,
> @@ -55,7 +57,7 @@ static int do_bootm_barebox(struct image_data *data)
>         if (ret)
>                 goto out;
> 
> -       barebox = start;
> +       barebox = ALIGN(start, ADRP_PAGE_MASK);

I'd suggest using PAGE_ALIGN here.

While I agree that the barebox image must be page aligned to be
correctly started, I wonder how it can happen that the address returned
from memory_bank_first_find_space() is not page aligned. Normally this
should be the start address of your DRAM. How comes this address is not
aligned in your case?

Sascha

-- 
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 |

Re: [PATCH] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction

[Thaison Phan]

Hi Sascha,

For the system I am running in, barebox is constrained to run within 4MB
of memory. Because of this, when arm_mem_barebox_image() calculates
the address to uncompress barebox to, it selects an address that ends up
being within ~2MB from the start of memory that was designated for barebox
to use. The malloc space ends at the start of the barebox image, and ends
up taking up to the start of memory. memory_bank_first_find_space ends
up finding the first available location to be after the .bss section which is
not aligned to 4KB. It seems like my case is a very unlikely corner case,
but I create a patch using PAGE_ALIGN as you suggested just in case it
may be useful.

Signed-off-by: Thaison Phan <tsphan42 at gmail.com>
---
 arch/arm/lib64/armlinux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/lib64/armlinux.c b/arch/arm/lib64/armlinux.c
index 8382ffdf1b..8b62131347 100644
--- a/arch/arm/lib64/armlinux.c
+++ b/arch/arm/lib64/armlinux.c
@@ -55,7 +55,7 @@ static int do_bootm_barebox(struct image_data *data)
  if (ret)
  goto out;

- barebox = start;
+ barebox = PAGE_ALIGN(start);

  ret = bootm_load_os(data, barebox);
  if (ret)
-- 
2.25.1

Thanks,
Thaison

On Thu, Feb 8, 2024 at 2:30 AM Sascha Hauer <sha@pengutronix.de> wrote:
>
> Hi Thaison,
>
> On Tue, Feb 06, 2024 at 09:37:47PM -0500, Thaison Phan wrote:
> > Hi,
> >
> > The aarch64 bootm image handler for barebox can choose a load address
> > that is not 4KB aligned. This can result in unexpected behavior with
> > the ADRP instruction that is available in 64 bit ARM architectures.
> > ADRP forms a PC-relative address to a 4KB page where the bottom 12
> > bits of the current PC will be masked out. When the load address of
> > the barebox image is not 4KB aligned ADRP can end up forming an
> > address that starts from an invalid page of memory or the wrong page
> > of memory that was expected to be formed. The following patch aligns
> > the load address for the next barebox image to be 4KB aligned to
> > accommodate the ADRP instruction.
> >
> > Thanks,
> > Thaison
> >
> > ---
> >  arch/arm/lib64/armlinux.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/lib64/armlinux.c b/arch/arm/lib64/armlinux.c
> > index 8382ffdf1b..d66963dae5 100644
> > --- a/arch/arm/lib64/armlinux.c
> > +++ b/arch/arm/lib64/armlinux.c
> > @@ -43,6 +43,8 @@ static struct image_handler aarch64_fit_handler = {
> >         .filetype = filetype_oftree,
> >  };
> >
> > +#define ADRP_PAGE_MASK 0x1000
> > +
> >  static int do_bootm_barebox(struct image_data *data)
> >  {
> >         void (*fn)(unsigned long x0, unsigned long x1, unsigned long x2,
> > @@ -55,7 +57,7 @@ static int do_bootm_barebox(struct image_data *data)
> >         if (ret)
> >                 goto out;
> >
> > -       barebox = start;
> > +       barebox = ALIGN(start, ADRP_PAGE_MASK);
>
> I'd suggest using PAGE_ALIGN here.
>
> While I agree that the barebox image must be page aligned to be
> correctly started, I wonder how it can happen that the address returned
> from memory_bank_first_find_space() is not page aligned. Normally this
> should be the start address of your DRAM. How comes this address is not
> aligned in your case?
>
> Sascha
>
> --
> 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 |

Re: [PATCH] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction

[Sascha Hauer]

On Tue, 06 Feb 2024 21:37:47 -0500, Thaison Phan wrote:
> The aarch64 bootm image handler for barebox can choose a load address
> that is not 4KB aligned. This can result in unexpected behavior with
> the ADRP instruction that is available in 64 bit ARM architectures.
> ADRP forms a PC-relative address to a 4KB page where the bottom 12
> bits of the current PC will be masked out. When the load address of
> the barebox image is not 4KB aligned ADRP can end up forming an
> address that starts from an invalid page of memory or the wrong page
> of memory that was expected to be formed. The following patch aligns
> the load address for the next barebox image to be 4KB aligned to
> accommodate the ADRP instruction.
> 
> [...]

Applied, thanks!

[1/1] Patch to fix bootm barebox load address alignment to acomodate ADRP instruction
      https://git.pengutronix.de/cgit/barebox/commit/?id=af694bdae717 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>