[PATCH] MIPS: boot/compressed: Use array reference for image bounds

[PATCH] MIPS: boot/compressed: Use array reference for image bounds

[Kees Cook]

As done with other image addresses in other architectures, use an
explicit flexible array instead of "address of char", which can trip
bounds checking done by the compiler. Found when building with
-Warray-bounds:

In file included from ./include/linux/byteorder/little_endian.h:5,
                 from ./arch/mips/include/uapi/asm/byteorder.h:15,
                 from ./arch/mips/include/asm/bitops.h:21,
                 from ./include/linux/bitops.h:33,
                 from ./include/linux/kernel.h:22,
                 from arch/mips/boot/compressed/decompress.c:13:
arch/mips/boot/compressed/decompress.c: In function 'decompress_kernel':
./include/asm-generic/unaligned.h:14:8: warning: array subscript -1 is outside array bounds of 'unsigned char[1]' [-Warray-bounds]
   14 |  __pptr->x;        \
      |  ~~~~~~^~~
./include/uapi/linux/byteorder/little_endian.h:35:51: note: in definition of macro '__le32_to_cpu'
   35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
      |                                                   ^
./include/asm-generic/unaligned.h:32:21: note: in expansion of macro '__get_unaligned_t'
   32 |  return le32_to_cpu(__get_unaligned_t(__le32, p));
      |                     ^~~~~~~~~~~~~~~~~
arch/mips/boot/compressed/decompress.c:29:37: note: while referencing '__image_end'
   29 | extern unsigned char __image_begin, __image_end;
      |                                     ^~~~~~~~~~~

Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: linux-mips@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/mips/boot/compressed/decompress.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
index aae1346a509a..5b38a802e101 100644
--- a/arch/mips/boot/compressed/decompress.c
+++ b/arch/mips/boot/compressed/decompress.c
@@ -26,7 +26,7 @@ unsigned long free_mem_ptr;
 unsigned long free_mem_end_ptr;
 
 /* The linker tells us where the image is. */
-extern unsigned char __image_begin, __image_end;
+extern unsigned char __image_begin[], __image_end[];
 
 /* debug interfaces  */
 #ifdef CONFIG_DEBUG_ZBOOT
@@ -91,9 +91,9 @@ void decompress_kernel(unsigned long boot_heap_start)
 {
 	unsigned long zimage_start, zimage_size;
 
-	zimage_start = (unsigned long)(&__image_begin);
-	zimage_size = (unsigned long)(&__image_end) -
-	    (unsigned long)(&__image_begin);
+	zimage_start = (unsigned long)(__image_begin);
+	zimage_size = (unsigned long)(__image_end) -
+	    (unsigned long)(__image_begin);
 
 	puts("zimage at:     ");
 	puthex(zimage_start);
@@ -121,7 +121,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 = get_unaligned_le32((void *)&__image_end - 4);
+		image_size = get_unaligned_le32((void *)__image_end - 4);
 
 		/* The device tree's address must be properly aligned  */
 		image_size = ALIGN(image_size, STRUCT_ALIGNMENT);
-- 
2.32.0

Re: [PATCH] MIPS: boot/compressed: Use array reference for image bounds

[Gustavo A. R. Silva]

On Wed, Mar 09, 2022 at 10:50:32AM -0800, Kees Cook wrote:
> As done with other image addresses in other architectures, use an
> explicit flexible array instead of "address of char", which can trip
> bounds checking done by the compiler. Found when building with
> -Warray-bounds:
> 
> In file included from ./include/linux/byteorder/little_endian.h:5,
>                  from ./arch/mips/include/uapi/asm/byteorder.h:15,
>                  from ./arch/mips/include/asm/bitops.h:21,
>                  from ./include/linux/bitops.h:33,
>                  from ./include/linux/kernel.h:22,
>                  from arch/mips/boot/compressed/decompress.c:13:
> arch/mips/boot/compressed/decompress.c: In function 'decompress_kernel':
> ./include/asm-generic/unaligned.h:14:8: warning: array subscript -1 is outside array bounds of 'unsigned char[1]' [-Warray-bounds]
>    14 |  __pptr->x;        \
>       |  ~~~~~~^~~
> ./include/uapi/linux/byteorder/little_endian.h:35:51: note: in definition of macro '__le32_to_cpu'
>    35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
>       |                                                   ^
> ./include/asm-generic/unaligned.h:32:21: note: in expansion of macro '__get_unaligned_t'
>    32 |  return le32_to_cpu(__get_unaligned_t(__le32, p));
>       |                     ^~~~~~~~~~~~~~~~~
> arch/mips/boot/compressed/decompress.c:29:37: note: while referencing '__image_end'
>    29 | extern unsigned char __image_begin, __image_end;
>       |                                     ^~~~~~~~~~~
> 
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: linux-mips@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Thanks
--
Gustavo

> ---
>  arch/mips/boot/compressed/decompress.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/mips/boot/compressed/decompress.c b/arch/mips/boot/compressed/decompress.c
> index aae1346a509a..5b38a802e101 100644
> --- a/arch/mips/boot/compressed/decompress.c
> +++ b/arch/mips/boot/compressed/decompress.c
> @@ -26,7 +26,7 @@ unsigned long free_mem_ptr;
>  unsigned long free_mem_end_ptr;
>  
>  /* The linker tells us where the image is. */
> -extern unsigned char __image_begin, __image_end;
> +extern unsigned char __image_begin[], __image_end[];
>  
>  /* debug interfaces  */
>  #ifdef CONFIG_DEBUG_ZBOOT
> @@ -91,9 +91,9 @@ void decompress_kernel(unsigned long boot_heap_start)
>  {
>  	unsigned long zimage_start, zimage_size;
>  
> -	zimage_start = (unsigned long)(&__image_begin);
> -	zimage_size = (unsigned long)(&__image_end) -
> -	    (unsigned long)(&__image_begin);
> +	zimage_start = (unsigned long)(__image_begin);
> +	zimage_size = (unsigned long)(__image_end) -
> +	    (unsigned long)(__image_begin);
>  
>  	puts("zimage at:     ");
>  	puthex(zimage_start);
> @@ -121,7 +121,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 = get_unaligned_le32((void *)&__image_end - 4);
> +		image_size = get_unaligned_le32((void *)__image_end - 4);
>  
>  		/* The device tree's address must be properly aligned  */
>  		image_size = ALIGN(image_size, STRUCT_ALIGNMENT);
> -- 
> 2.32.0
> 

Re: [PATCH] MIPS: boot/compressed: Use array reference for image bounds

[Thomas Bogendoerfer]

On Wed, Mar 09, 2022 at 10:50:32AM -0800, Kees Cook wrote:
> As done with other image addresses in other architectures, use an
> explicit flexible array instead of "address of char", which can trip
> bounds checking done by the compiler. Found when building with
> -Warray-bounds:
> 
> In file included from ./include/linux/byteorder/little_endian.h:5,
>                  from ./arch/mips/include/uapi/asm/byteorder.h:15,
>                  from ./arch/mips/include/asm/bitops.h:21,
>                  from ./include/linux/bitops.h:33,
>                  from ./include/linux/kernel.h:22,
>                  from arch/mips/boot/compressed/decompress.c:13:
> arch/mips/boot/compressed/decompress.c: In function 'decompress_kernel':
> ./include/asm-generic/unaligned.h:14:8: warning: array subscript -1 is outside array bounds of 'unsigned char[1]' [-Warray-bounds]
>    14 |  __pptr->x;        \
>       |  ~~~~~~^~~
> ./include/uapi/linux/byteorder/little_endian.h:35:51: note: in definition of macro '__le32_to_cpu'
>    35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
>       |                                                   ^
> ./include/asm-generic/unaligned.h:32:21: note: in expansion of macro '__get_unaligned_t'
>    32 |  return le32_to_cpu(__get_unaligned_t(__le32, p));
>       |                     ^~~~~~~~~~~~~~~~~
> arch/mips/boot/compressed/decompress.c:29:37: note: while referencing '__image_end'
>    29 | extern unsigned char __image_begin, __image_end;
>       |                                     ^~~~~~~~~~~
> 
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: linux-mips@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/mips/boot/compressed/decompress.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

applied to mips-next.

Thomas.

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