[PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Paul Cercueil]

The compressed payload is not necesarily 4-byte aligned, at least when
compiling with Clang. In that case, the 4-byte value appended to the
compressed payload that corresponds to the uncompressed kernel image
size must be read using get_unaligned_le().

This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic
JZ4770 board).

Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel")
Cc: <stable@vger.kernel.org> # v4.7
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---
 arch/mips/boot/compressed/decompress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
index c61c641674e6..47c07990432b 100644
--- a/arch/mips/boot/compressed/decompress.c
+++ b/arch/mips/boot/compressed/decompress.c
@@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start)
 		dtb_size = fdt_totalsize((void *)&__appended_dtb);
 
 		/* last four bytes is always image size in little endian */
-		image_size = le32_to_cpup((void *)&__image_end - 4);
+		image_size = get_unaligned_le32((void *)&__image_end - 4);
 
 		/* copy dtb to where the booted kernel will expect it */
 		memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size,
-- 
2.29.2

Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Nick Desaulniers]

On Wed, Dec 16, 2020 at 3:40 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> The compressed payload is not necesarily 4-byte aligned, at least when
> compiling with Clang. In that case, the 4-byte value appended to the
> compressed payload that corresponds to the uncompressed kernel image
> size must be read using get_unaligned_le().

Should it be get_unaligned_le32()?

>
> This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic
> JZ4770 board).
>
> Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel")
> Cc: <stable@vger.kernel.org> # v4.7
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

Hi Paul, thanks for the patch (and for testing with Clang)!
Alternatively, we could re-align __image_end to the next 4B multiple
via:

diff --git a/arch/mips/boot/compressed/ld.script
b/arch/mips/boot/compressed/ld.script
index 0ebb667274d6..349919eff5fb 100644
--- a/arch/mips/boot/compressed/ld.script
+++ b/arch/mips/boot/compressed/ld.script
@@ -27,6 +27,7 @@ SECTIONS
                /* Put the compressed image here */
                __image_begin = .;
                *(.image)
+               . = ALIGN(4);
                __image_end = .;
                CONSTRUCTORS
                . = ALIGN(16);

The tradeoff being up to 3 wasted bytes of padding in the compressed
image, vs fetching one value slower (assuming unaligned loads are
slower than aligned loads MIPS, IDK).  I doubt decompress_kernel is
called repeatedly, so let's take the byte saving approach of yours by
using unaligned loads!

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  arch/mips/boot/compressed/decompress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
> index c61c641674e6..47c07990432b 100644
> --- a/arch/mips/boot/compressed/decompress.c
> +++ b/arch/mips/boot/compressed/decompress.c
> @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start)
>                 dtb_size = fdt_totalsize((void *)&__appended_dtb);
>
>                 /* last four bytes is always image size in little endian */
> -               image_size = le32_to_cpup((void *)&__image_end - 4);
> +               image_size = get_unaligned_le32((void *)&__image_end - 4);
>
>                 /* copy dtb to where the booted kernel will expect it */
>                 memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + image_size,
> --
> 2.29.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Paul Cercueil]

Hi Nick,

Le mer. 16 déc. 2020 à 18:08, Nick Desaulniers 
<ndesaulniers@google.com> a écrit :
> On Wed, Dec 16, 2020 at 3:40 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>>  The compressed payload is not necesarily 4-byte aligned, at least 
>> when
>>  compiling with Clang. In that case, the 4-byte value appended to the
>>  compressed payload that corresponds to the uncompressed kernel image
>>  size must be read using get_unaligned_le().
> 
> Should it be get_unaligned_le32()?

Indeed.

>> 
>>  This fixes Clang-built kernels not booting on MIPS (tested on a 
>> Ingenic
>>  JZ4770 board).
>> 
>>  Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of 
>> the kernel")
>>  Cc: <stable@vger.kernel.org> # v4.7
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> Hi Paul, thanks for the patch (and for testing with Clang)!
> Alternatively, we could re-align __image_end to the next 4B multiple
> via:
> 
> diff --git a/arch/mips/boot/compressed/ld.script
> b/arch/mips/boot/compressed/ld.script
> index 0ebb667274d6..349919eff5fb 100644
> --- a/arch/mips/boot/compressed/ld.script
> +++ b/arch/mips/boot/compressed/ld.script
> @@ -27,6 +27,7 @@ SECTIONS
>                 /* Put the compressed image here */
>                 __image_begin = .;
>                 *(.image)
> +               . = ALIGN(4);
>                 __image_end = .;
>                 CONSTRUCTORS
>                 . = ALIGN(16);

Actually that would not work (I did try that), since the 4-byte size 
appended to the compressed payload is inside the *(.image) section. The 
code that appends it (in scripts/Makefile.lib, I think) doesn't seem to 
take care about aligning it to a 4-byte offset. I have no idea why it 
does with GCC and doesn't with Clang, and I have no idea why the 
compressed payload's size isn't aligned either.

> The tradeoff being up to 3 wasted bytes of padding in the compressed
> image, vs fetching one value slower (assuming unaligned loads are
> slower than aligned loads MIPS, IDK).  I doubt decompress_kernel is
> called repeatedly, so let's take the byte saving approach of yours by
> using unaligned loads!
> 
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks.

Cheers,
-Paul

>>  ---
>>   arch/mips/boot/compressed/decompress.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>>  diff --git a/arch/mips/boot/compressed/decompress.c 
>> b/arch/mips/boot/compressed/decompress.c
>>  index c61c641674e6..47c07990432b 100644
>>  --- a/arch/mips/boot/compressed/decompress.c
>>  +++ b/arch/mips/boot/compressed/decompress.c
>>  @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long 
>> boot_heap_start)
>>                  dtb_size = fdt_totalsize((void *)&__appended_dtb);
>> 
>>                  /* last four bytes is always image size in little 
>> endian */
>>  -               image_size = le32_to_cpup((void *)&__image_end - 4);
>>  +               image_size = get_unaligned_le32((void 
>> *)&__image_end - 4);
>> 
>>                  /* copy dtb to where the booted kernel will expect 
>> it */
>>                  memcpy((void *)VMLINUX_LOAD_ADDRESS_ULL + 
>> image_size,
>>  --
>>  2.29.2
>> 
> 
> 
> --
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Philippe Mathieu-Daudé]

On Thu, Dec 17, 2020 at 12:41 AM Paul Cercueil <paul@crapouillou.net> wrote:
>
> The compressed payload is not necesarily 4-byte aligned, at least when
> compiling with Clang. In that case, the 4-byte value appended to the
> compressed payload that corresponds to the uncompressed kernel image
> size must be read using get_unaligned_le().
>
> This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic
> JZ4770 board).
>
> Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel")
> Cc: <stable@vger.kernel.org> # v4.7
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/compressed/decompress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Thomas Bogendoerfer]

On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote:
> The compressed payload is not necesarily 4-byte aligned, at least when
> compiling with Clang. In that case, the 4-byte value appended to the
> compressed payload that corresponds to the uncompressed kernel image
> size must be read using get_unaligned_le().
> 
> This fixes Clang-built kernels not booting on MIPS (tested on a Ingenic
> JZ4770 board).
> 
> Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of the kernel")
> Cc: <stable@vger.kernel.org> # v4.7
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>  arch/mips/boot/compressed/decompress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
> index c61c641674e6..47c07990432b 100644
> --- a/arch/mips/boot/compressed/decompress.c
> +++ b/arch/mips/boot/compressed/decompress.c
> @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long boot_heap_start)
>  		dtb_size = fdt_totalsize((void *)&__appended_dtb);
>  
>  		/* last four bytes is always image size in little endian */
> -		image_size = le32_to_cpup((void *)&__image_end - 4);
> +		image_size = get_unaligned_le32((void *)&__image_end - 4);

gives me following error

arch/mips/boot/compressed/decompress.c:120:16: error: implicit declaration of function ‘get_unaligned_le32’ [-Werror=implicit-function-declaration]
   image_size = get_unaligned_le32((void *)&__image_end - 4);

I've added

#include <asm/unaligned.h>

which fixes the compile error, but I'm wondering why the patch compiled
for you ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: boot: Fix unaligned access with CONFIG_MIPS_RAW_APPENDED_DTB

[Paul Cercueil]

Hi Thomas,

Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer 
<tsbogend@alpha.franken.de> a écrit :
> On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote:
>>  The compressed payload is not necesarily 4-byte aligned, at least 
>> when
>>  compiling with Clang. In that case, the 4-byte value appended to the
>>  compressed payload that corresponds to the uncompressed kernel image
>>  size must be read using get_unaligned_le().
>> 
>>  This fixes Clang-built kernels not booting on MIPS (tested on a 
>> Ingenic
>>  JZ4770 board).
>> 
>>  Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of 
>> the kernel")
>>  Cc: <stable@vger.kernel.org> # v4.7
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  ---
>>   arch/mips/boot/compressed/decompress.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>>  diff --git a/arch/mips/boot/compressed/decompress.c 
>> b/arch/mips/boot/compressed/decompress.c
>>  index c61c641674e6..47c07990432b 100644
>>  --- a/arch/mips/boot/compressed/decompress.c
>>  +++ b/arch/mips/boot/compressed/decompress.c
>>  @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long 
>> boot_heap_start)
>>   		dtb_size = fdt_totalsize((void *)&__appended_dtb);
>> 
>>   		/* last four bytes is always image size in little endian */
>>  -		image_size = le32_to_cpup((void *)&__image_end - 4);
>>  +		image_size = get_unaligned_le32((void *)&__image_end - 4);
> 
> gives me following error
> 
> arch/mips/boot/compressed/decompress.c:120:16: error: implicit 
> declaration of function ‘get_unaligned_le32’ 
> [-Werror=implicit-function-declaration]
>    image_size = get_unaligned_le32((void *)&__image_end - 4);
> 
> I've added
> 
> #include <asm/unaligned.h>
> 
> which fixes the compile error, but I'm wondering why the patch 
> compiled
> for you ?

No idea - but it does compile fine without the include here. Probably a 
defconfig difference.

Cheers,
-Paul

Re: [PATCH] MIPS: boot: Fix unaligned access with?? CONFIG_MIPS_RAW_APPENDED_DTB

[Thomas Bogendoerfer]

On Mon, Dec 28, 2020 at 10:30:36PM +0000, Paul Cercueil wrote:
> Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer
> <tsbogend@alpha.franken.de> a écrit :
> > On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote:
> > >  The compressed payload is not necesarily 4-byte aligned, at least
> > > when
> > >  compiling with Clang. In that case, the 4-byte value appended to the
> > >  compressed payload that corresponds to the uncompressed kernel image
> > >  size must be read using get_unaligned_le().
> > > 
> > >  This fixes Clang-built kernels not booting on MIPS (tested on a
> > > Ingenic
> > >  JZ4770 board).
> > > 
> > >  Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the end of
> > > the kernel")
> > >  Cc: <stable@vger.kernel.org> # v4.7
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   arch/mips/boot/compressed/decompress.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > >  diff --git a/arch/mips/boot/compressed/decompress.c
> > > b/arch/mips/boot/compressed/decompress.c
> > >  index c61c641674e6..47c07990432b 100644
> > >  --- a/arch/mips/boot/compressed/decompress.c
> > >  +++ b/arch/mips/boot/compressed/decompress.c
> > >  @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long
> > > boot_heap_start)
> > >   		dtb_size = fdt_totalsize((void *)&__appended_dtb);
> > > 
> > >   		/* last four bytes is always image size in little endian */
> > >  -		image_size = le32_to_cpup((void *)&__image_end - 4);
> > >  +		image_size = get_unaligned_le32((void *)&__image_end - 4);
> > 
> > gives me following error
> > 
> > arch/mips/boot/compressed/decompress.c:120:16: error: implicit
> > declaration of function ‘get_unaligned_le32’
> > [-Werror=implicit-function-declaration]
> >    image_size = get_unaligned_le32((void *)&__image_end - 4);
> > 
> > I've added
> > 
> > #include <asm/unaligned.h>
> > 
> > which fixes the compile error, but I'm wondering why the patch compiled
> > for you ?
> 
> No idea - but it does compile fine without the include here. Probably a
> defconfig difference.

# CONFIG_KERNEL_LZO is not set
# CONFIG_KERNEL_LZ4 is not set

this makes the difference. Both decompress.c files include asm/unaligned.h.

I've added the #include, fixed the get_unaligned_le32 in the description
and applied it to mips-fixes.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: boot: Fix unaligned access with?? CONFIG_MIPS_RAW_APPENDED_DTB

[Paul Cercueil]

Hi Thomas,

Le mar. 29 déc. 2020 à 16:08, Thomas Bogendoerfer 
<tsbogend@alpha.franken.de> a écrit :
> On Mon, Dec 28, 2020 at 10:30:36PM +0000, Paul Cercueil wrote:
>>  Le lun. 28 déc. 2020 à 23:25, Thomas Bogendoerfer
>>  <tsbogend@alpha.franken.de> a écrit :
>>  > On Wed, Dec 16, 2020 at 11:39:56PM +0000, Paul Cercueil wrote:
>>  > >  The compressed payload is not necesarily 4-byte aligned, at 
>> least
>>  > > when
>>  > >  compiling with Clang. In that case, the 4-byte value appended 
>> to the
>>  > >  compressed payload that corresponds to the uncompressed kernel 
>> image
>>  > >  size must be read using get_unaligned_le().
>>  > >
>>  > >  This fixes Clang-built kernels not booting on MIPS (tested on a
>>  > > Ingenic
>>  > >  JZ4770 board).
>>  > >
>>  > >  Fixes: b8f54f2cde78 ("MIPS: ZBOOT: copy appended dtb to the 
>> end of
>>  > > the kernel")
>>  > >  Cc: <stable@vger.kernel.org> # v4.7
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  > >  ---
>>  > >   arch/mips/boot/compressed/decompress.c | 2 +-
>>  > >   1 file changed, 1 insertion(+), 1 deletion(-)
>>  > >
>>  > >  diff --git a/arch/mips/boot/compressed/decompress.c
>>  > > b/arch/mips/boot/compressed/decompress.c
>>  > >  index c61c641674e6..47c07990432b 100644
>>  > >  --- a/arch/mips/boot/compressed/decompress.c
>>  > >  +++ b/arch/mips/boot/compressed/decompress.c
>>  > >  @@ -117,7 +117,7 @@ void decompress_kernel(unsigned long
>>  > > boot_heap_start)
>>  > >   		dtb_size = fdt_totalsize((void *)&__appended_dtb);
>>  > >
>>  > >   		/* last four bytes is always image size in little endian */
>>  > >  -		image_size = le32_to_cpup((void *)&__image_end - 4);
>>  > >  +		image_size = get_unaligned_le32((void *)&__image_end - 4);
>>  >
>>  > gives me following error
>>  >
>>  > arch/mips/boot/compressed/decompress.c:120:16: error: implicit
>>  > declaration of function ‘get_unaligned_le32’
>>  > [-Werror=implicit-function-declaration]
>>  >    image_size = get_unaligned_le32((void *)&__image_end - 4);
>>  >
>>  > I've added
>>  >
>>  > #include <asm/unaligned.h>
>>  >
>>  > which fixes the compile error, but I'm wondering why the patch 
>> compiled
>>  > for you ?
>> 
>>  No idea - but it does compile fine without the include here. 
>> Probably a
>>  defconfig difference.
> 
> # CONFIG_KERNEL_LZO is not set
> # CONFIG_KERNEL_LZ4 is not set
> 
> this makes the difference. Both decompress.c files include 
> asm/unaligned.h.
> 
> I've added the #include, fixed the get_unaligned_le32 in the 
> description
> and applied it to mips-fixes.

Alright, great! Thanks!

Cheers,
-Paul