All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] x86/boot: Wrap literal addresses in absolute_pointer()
@ 2022-02-27 19:59 Kees Cook
  2022-02-27 20:25 ` Guenter Roeck
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Kees Cook @ 2022-02-27 19:59 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Kees Cook, Guenter Roeck, x86, linux-kernel, linux-hardening

GCC 11 (incorrectly[1]) assumes that literal values cast to (void *)
should be treated like a NULL pointer with an offset, and raises
diagnostics when doing bounds checking under -Warray-bounds. GCC 12
got "smarter" about finding these:

In function 'rdfs8',
    inlined from 'vga_recalc_vertical' at /srv/code/arch/x86/boot/video-mode.c:124:29,
    inlined from 'set_mode' at /srv/code/arch/x86/boot/video-mode.c:163:3:
/srv/code/arch/x86/boot/boot.h:114:9: warning: array subscript 0 is outside array bounds of 'u8[0]' {aka 'unsigned char[]'} [-Warray-bounds]
  114 |         asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
      |         ^~~

This has been solved in other places[2] already by using the recently
added absolute_pointer() macro. Do the same here.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
[2] https://lore.kernel.org/all/20210912160149.2227137-1-linux@roeck-us.net/

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: x86@kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/boot/boot.h | 36 ++++++++++++++++++++++++------------
 arch/x86/boot/main.c |  2 +-
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 34c9dbb6a47d..686a9d75a0e4 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -110,66 +110,78 @@ typedef unsigned int addr_t;
 
 static inline u8 rdfs8(addr_t addr)
 {
+	u8 *ptr = (u8 *)absolute_pointer(addr);
 	u8 v;
-	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
+	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*ptr));
 	return v;
 }
 static inline u16 rdfs16(addr_t addr)
 {
+	u16 *ptr = (u16 *)absolute_pointer(addr);
 	u16 v;
-	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
+	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 static inline u32 rdfs32(addr_t addr)
 {
+	u32 *ptr = (u32 *)absolute_pointer(addr);
 	u32 v;
-	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
+	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 
 static inline void wrfs8(u8 v, addr_t addr)
 {
-	asm volatile("movb %1,%%fs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
+	u8 *ptr = (u8 *)absolute_pointer(addr);
+	asm volatile("movb %1,%%fs:%0" : "+m" (*ptr) : "qi" (v));
 }
 static inline void wrfs16(u16 v, addr_t addr)
 {
-	asm volatile("movw %1,%%fs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
+	u16 *ptr = (u16 *)absolute_pointer(addr);
+	asm volatile("movw %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
 }
 static inline void wrfs32(u32 v, addr_t addr)
 {
-	asm volatile("movl %1,%%fs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
+	u32 *ptr = (u32 *)absolute_pointer(addr);
+	asm volatile("movl %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
 }
 
 static inline u8 rdgs8(addr_t addr)
 {
+	u8 *ptr = (u8 *)absolute_pointer(addr);
 	u8 v;
-	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
+	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*ptr));
 	return v;
 }
 static inline u16 rdgs16(addr_t addr)
 {
+	u16 *ptr = (u16 *)absolute_pointer(addr);
 	u16 v;
-	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
+	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 static inline u32 rdgs32(addr_t addr)
 {
+	u32 *ptr = (u32 *)absolute_pointer(addr);
 	u32 v;
-	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
+	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 
 static inline void wrgs8(u8 v, addr_t addr)
 {
-	asm volatile("movb %1,%%gs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
+	u8 *ptr = (u8 *)absolute_pointer(addr);
+	asm volatile("movb %1,%%gs:%0" : "+m" (*ptr) : "qi" (v));
 }
 static inline void wrgs16(u16 v, addr_t addr)
 {
-	asm volatile("movw %1,%%gs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
+	u16 *ptr = (u16 *)absolute_pointer(addr);
+	asm volatile("movw %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
 }
 static inline void wrgs32(u32 v, addr_t addr)
 {
-	asm volatile("movl %1,%%gs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
+	u32 *ptr = (u32 *)absolute_pointer(addr);
+	asm volatile("movl %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
 }
 
 /* Note: these only return true/false, not a signed return value! */
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index e3add857c2c9..c421af5a3cdc 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -33,7 +33,7 @@ static void copy_boot_params(void)
 		u16 cl_offset;
 	};
 	const struct old_cmdline * const oldcmd =
-		(const struct old_cmdline *)OLD_CL_ADDRESS;
+		absolute_pointer(OLD_CL_ADDRESS);
 
 	BUILD_BUG_ON(sizeof(boot_params) != 4096);
 	memcpy(&boot_params.hdr, &hdr, sizeof(hdr));
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] x86/boot: Wrap literal addresses in absolute_pointer()
  2022-02-27 19:59 [PATCH] x86/boot: Wrap literal addresses in absolute_pointer() Kees Cook
@ 2022-02-27 20:25 ` Guenter Roeck
  2022-05-19  5:41 ` Kees Cook
  2022-05-19 10:57 ` [tip: x86/build] " tip-bot2 for Kees Cook
  2 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-02-27 20:25 UTC (permalink / raw)
  To: Kees Cook, Thomas Gleixner; +Cc: x86, linux-kernel, linux-hardening

On 2/27/22 11:59, Kees Cook wrote:
> GCC 11 (incorrectly[1]) assumes that literal values cast to (void *)
> should be treated like a NULL pointer with an offset, and raises
> diagnostics when doing bounds checking under -Warray-bounds. GCC 12
> got "smarter" about finding these:
> 
> In function 'rdfs8',
>      inlined from 'vga_recalc_vertical' at /srv/code/arch/x86/boot/video-mode.c:124:29,
>      inlined from 'set_mode' at /srv/code/arch/x86/boot/video-mode.c:163:3:
> /srv/code/arch/x86/boot/boot.h:114:9: warning: array subscript 0 is outside array bounds of 'u8[0]' {aka 'unsigned char[]'} [-Warray-bounds]
>    114 |         asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
>        |         ^~~
> 
> This has been solved in other places[2] already by using the recently
> added absolute_pointer() macro. Do the same here.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> [2] https://lore.kernel.org/all/20210912160149.2227137-1-linux@roeck-us.net/
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: x86@kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   arch/x86/boot/boot.h | 36 ++++++++++++++++++++++++------------
>   arch/x86/boot/main.c |  2 +-
>   2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index 34c9dbb6a47d..686a9d75a0e4 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -110,66 +110,78 @@ typedef unsigned int addr_t;
>   
>   static inline u8 rdfs8(addr_t addr)
>   {
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
>   	u8 v;
> -	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
> +	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*ptr));
>   	return v;
>   }
>   static inline u16 rdfs16(addr_t addr)
>   {
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
>   	u16 v;
> -	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
> +	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
>   	return v;
>   }
>   static inline u32 rdfs32(addr_t addr)
>   {
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
>   	u32 v;
> -	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
> +	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
>   	return v;
>   }
>   
>   static inline void wrfs8(u8 v, addr_t addr)
>   {
> -	asm volatile("movb %1,%%fs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
> +	asm volatile("movb %1,%%fs:%0" : "+m" (*ptr) : "qi" (v));
>   }
>   static inline void wrfs16(u16 v, addr_t addr)
>   {
> -	asm volatile("movw %1,%%fs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
> +	asm volatile("movw %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
>   }
>   static inline void wrfs32(u32 v, addr_t addr)
>   {
> -	asm volatile("movl %1,%%fs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
> +	asm volatile("movl %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
>   }
>   
>   static inline u8 rdgs8(addr_t addr)
>   {
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
>   	u8 v;
> -	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
> +	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*ptr));
>   	return v;
>   }
>   static inline u16 rdgs16(addr_t addr)
>   {
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
>   	u16 v;
> -	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
> +	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
>   	return v;
>   }
>   static inline u32 rdgs32(addr_t addr)
>   {
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
>   	u32 v;
> -	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
> +	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
>   	return v;
>   }
>   
>   static inline void wrgs8(u8 v, addr_t addr)
>   {
> -	asm volatile("movb %1,%%gs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
> +	asm volatile("movb %1,%%gs:%0" : "+m" (*ptr) : "qi" (v));
>   }
>   static inline void wrgs16(u16 v, addr_t addr)
>   {
> -	asm volatile("movw %1,%%gs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
> +	asm volatile("movw %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
>   }
>   static inline void wrgs32(u32 v, addr_t addr)
>   {
> -	asm volatile("movl %1,%%gs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
> +	asm volatile("movl %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
>   }
>   
>   /* Note: these only return true/false, not a signed return value! */
> diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
> index e3add857c2c9..c421af5a3cdc 100644
> --- a/arch/x86/boot/main.c
> +++ b/arch/x86/boot/main.c
> @@ -33,7 +33,7 @@ static void copy_boot_params(void)
>   		u16 cl_offset;
>   	};
>   	const struct old_cmdline * const oldcmd =
> -		(const struct old_cmdline *)OLD_CL_ADDRESS;
> +		absolute_pointer(OLD_CL_ADDRESS);
>   
>   	BUILD_BUG_ON(sizeof(boot_params) != 4096);
>   	memcpy(&boot_params.hdr, &hdr, sizeof(hdr));


^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] x86/boot: Wrap literal addresses in absolute_pointer()
  2022-02-27 19:59 [PATCH] x86/boot: Wrap literal addresses in absolute_pointer() Kees Cook
  2022-02-27 20:25 ` Guenter Roeck
@ 2022-05-19  5:41 ` Kees Cook
  2022-05-19 10:57 ` [tip: x86/build] " tip-bot2 for Kees Cook
  2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2022-05-19  5:41 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Guenter Roeck, x86, linux-kernel, linux-hardening

On Sun, Feb 27, 2022 at 11:59:18AM -0800, Kees Cook wrote:
> GCC 11 (incorrectly[1]) assumes that literal values cast to (void *)
> should be treated like a NULL pointer with an offset, and raises
> diagnostics when doing bounds checking under -Warray-bounds. GCC 12
> got "smarter" about finding these:
> 
> In function 'rdfs8',
>     inlined from 'vga_recalc_vertical' at /srv/code/arch/x86/boot/video-mode.c:124:29,
>     inlined from 'set_mode' at /srv/code/arch/x86/boot/video-mode.c:163:3:
> /srv/code/arch/x86/boot/boot.h:114:9: warning: array subscript 0 is outside array bounds of 'u8[0]' {aka 'unsigned char[]'} [-Warray-bounds]
>   114 |         asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
>       |         ^~~
> 
> This has been solved in other places[2] already by using the recently
> added absolute_pointer() macro. Do the same here.
> 
> [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> [2] https://lore.kernel.org/all/20210912160149.2227137-1-linux@roeck-us.net/
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: x86@kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>

Thread ping -- this is still needed for GCC 12. Can an x86 maintainer
either take this or Ack it and I'll carry it?

Thanks!

-Kees

> ---
>  arch/x86/boot/boot.h | 36 ++++++++++++++++++++++++------------
>  arch/x86/boot/main.c |  2 +-
>  2 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
> index 34c9dbb6a47d..686a9d75a0e4 100644
> --- a/arch/x86/boot/boot.h
> +++ b/arch/x86/boot/boot.h
> @@ -110,66 +110,78 @@ typedef unsigned int addr_t;
>  
>  static inline u8 rdfs8(addr_t addr)
>  {
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
>  	u8 v;
> -	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
> +	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*ptr));
>  	return v;
>  }
>  static inline u16 rdfs16(addr_t addr)
>  {
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
>  	u16 v;
> -	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
> +	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
>  	return v;
>  }
>  static inline u32 rdfs32(addr_t addr)
>  {
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
>  	u32 v;
> -	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
> +	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
>  	return v;
>  }
>  
>  static inline void wrfs8(u8 v, addr_t addr)
>  {
> -	asm volatile("movb %1,%%fs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
> +	asm volatile("movb %1,%%fs:%0" : "+m" (*ptr) : "qi" (v));
>  }
>  static inline void wrfs16(u16 v, addr_t addr)
>  {
> -	asm volatile("movw %1,%%fs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
> +	asm volatile("movw %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
>  }
>  static inline void wrfs32(u32 v, addr_t addr)
>  {
> -	asm volatile("movl %1,%%fs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
> +	asm volatile("movl %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
>  }
>  
>  static inline u8 rdgs8(addr_t addr)
>  {
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
>  	u8 v;
> -	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
> +	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*ptr));
>  	return v;
>  }
>  static inline u16 rdgs16(addr_t addr)
>  {
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
>  	u16 v;
> -	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
> +	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
>  	return v;
>  }
>  static inline u32 rdgs32(addr_t addr)
>  {
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
>  	u32 v;
> -	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
> +	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
>  	return v;
>  }
>  
>  static inline void wrgs8(u8 v, addr_t addr)
>  {
> -	asm volatile("movb %1,%%gs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
> +	u8 *ptr = (u8 *)absolute_pointer(addr);
> +	asm volatile("movb %1,%%gs:%0" : "+m" (*ptr) : "qi" (v));
>  }
>  static inline void wrgs16(u16 v, addr_t addr)
>  {
> -	asm volatile("movw %1,%%gs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
> +	u16 *ptr = (u16 *)absolute_pointer(addr);
> +	asm volatile("movw %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
>  }
>  static inline void wrgs32(u32 v, addr_t addr)
>  {
> -	asm volatile("movl %1,%%gs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
> +	u32 *ptr = (u32 *)absolute_pointer(addr);
> +	asm volatile("movl %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
>  }
>  
>  /* Note: these only return true/false, not a signed return value! */
> diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
> index e3add857c2c9..c421af5a3cdc 100644
> --- a/arch/x86/boot/main.c
> +++ b/arch/x86/boot/main.c
> @@ -33,7 +33,7 @@ static void copy_boot_params(void)
>  		u16 cl_offset;
>  	};
>  	const struct old_cmdline * const oldcmd =
> -		(const struct old_cmdline *)OLD_CL_ADDRESS;
> +		absolute_pointer(OLD_CL_ADDRESS);
>  
>  	BUILD_BUG_ON(sizeof(boot_params) != 4096);
>  	memcpy(&boot_params.hdr, &hdr, sizeof(hdr));
> -- 
> 2.32.0
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [tip: x86/build] x86/boot: Wrap literal addresses in absolute_pointer()
  2022-02-27 19:59 [PATCH] x86/boot: Wrap literal addresses in absolute_pointer() Kees Cook
  2022-02-27 20:25 ` Guenter Roeck
  2022-05-19  5:41 ` Kees Cook
@ 2022-05-19 10:57 ` tip-bot2 for Kees Cook
  2 siblings, 0 replies; 4+ messages in thread
From: tip-bot2 for Kees Cook @ 2022-05-19 10:57 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Kees Cook, Borislav Petkov, Guenter Roeck, x86, linux-kernel

The following commit has been merged into the x86/build branch of tip:

Commit-ID:     aeb84412037b89e06f45e382f044da6f200e12f8
Gitweb:        https://git.kernel.org/tip/aeb84412037b89e06f45e382f044da6f200e12f8
Author:        Kees Cook <keescook@chromium.org>
AuthorDate:    Sun, 27 Feb 2022 11:59:18 -08:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 19 May 2022 12:47:30 +02:00

x86/boot: Wrap literal addresses in absolute_pointer()

GCC 11 (incorrectly[1]) assumes that literal values cast to (void *)
should be treated like a NULL pointer with an offset, and raises
diagnostics when doing bounds checking under -Warray-bounds. GCC 12
got "smarter" about finding these:

  In function 'rdfs8',
      inlined from 'vga_recalc_vertical' at /srv/code/arch/x86/boot/video-mode.c:124:29,
      inlined from 'set_mode' at /srv/code/arch/x86/boot/video-mode.c:163:3:
  /srv/code/arch/x86/boot/boot.h:114:9: warning: array subscript 0 is outside array bounds of 'u8[0]' {aka 'unsigned char[]'} [-Warray-bounds]
    114 |         asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
        |         ^~~

This has been solved in other places[2] already by using the recently
added absolute_pointer() macro. Do the same here.

  [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
  [2] https://lore.kernel.org/all/20210912160149.2227137-1-linux@roeck-us.net/

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20220227195918.705219-1-keescook@chromium.org
---
 arch/x86/boot/boot.h | 36 ++++++++++++++++++++++++------------
 arch/x86/boot/main.c |  2 +-
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 34c9dbb..686a9d7 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -110,66 +110,78 @@ typedef unsigned int addr_t;
 
 static inline u8 rdfs8(addr_t addr)
 {
+	u8 *ptr = (u8 *)absolute_pointer(addr);
 	u8 v;
-	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
+	asm volatile("movb %%fs:%1,%0" : "=q" (v) : "m" (*ptr));
 	return v;
 }
 static inline u16 rdfs16(addr_t addr)
 {
+	u16 *ptr = (u16 *)absolute_pointer(addr);
 	u16 v;
-	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
+	asm volatile("movw %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 static inline u32 rdfs32(addr_t addr)
 {
+	u32 *ptr = (u32 *)absolute_pointer(addr);
 	u32 v;
-	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
+	asm volatile("movl %%fs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 
 static inline void wrfs8(u8 v, addr_t addr)
 {
-	asm volatile("movb %1,%%fs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
+	u8 *ptr = (u8 *)absolute_pointer(addr);
+	asm volatile("movb %1,%%fs:%0" : "+m" (*ptr) : "qi" (v));
 }
 static inline void wrfs16(u16 v, addr_t addr)
 {
-	asm volatile("movw %1,%%fs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
+	u16 *ptr = (u16 *)absolute_pointer(addr);
+	asm volatile("movw %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
 }
 static inline void wrfs32(u32 v, addr_t addr)
 {
-	asm volatile("movl %1,%%fs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
+	u32 *ptr = (u32 *)absolute_pointer(addr);
+	asm volatile("movl %1,%%fs:%0" : "+m" (*ptr) : "ri" (v));
 }
 
 static inline u8 rdgs8(addr_t addr)
 {
+	u8 *ptr = (u8 *)absolute_pointer(addr);
 	u8 v;
-	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*(u8 *)addr));
+	asm volatile("movb %%gs:%1,%0" : "=q" (v) : "m" (*ptr));
 	return v;
 }
 static inline u16 rdgs16(addr_t addr)
 {
+	u16 *ptr = (u16 *)absolute_pointer(addr);
 	u16 v;
-	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*(u16 *)addr));
+	asm volatile("movw %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 static inline u32 rdgs32(addr_t addr)
 {
+	u32 *ptr = (u32 *)absolute_pointer(addr);
 	u32 v;
-	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*(u32 *)addr));
+	asm volatile("movl %%gs:%1,%0" : "=r" (v) : "m" (*ptr));
 	return v;
 }
 
 static inline void wrgs8(u8 v, addr_t addr)
 {
-	asm volatile("movb %1,%%gs:%0" : "+m" (*(u8 *)addr) : "qi" (v));
+	u8 *ptr = (u8 *)absolute_pointer(addr);
+	asm volatile("movb %1,%%gs:%0" : "+m" (*ptr) : "qi" (v));
 }
 static inline void wrgs16(u16 v, addr_t addr)
 {
-	asm volatile("movw %1,%%gs:%0" : "+m" (*(u16 *)addr) : "ri" (v));
+	u16 *ptr = (u16 *)absolute_pointer(addr);
+	asm volatile("movw %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
 }
 static inline void wrgs32(u32 v, addr_t addr)
 {
-	asm volatile("movl %1,%%gs:%0" : "+m" (*(u32 *)addr) : "ri" (v));
+	u32 *ptr = (u32 *)absolute_pointer(addr);
+	asm volatile("movl %1,%%gs:%0" : "+m" (*ptr) : "ri" (v));
 }
 
 /* Note: these only return true/false, not a signed return value! */
diff --git a/arch/x86/boot/main.c b/arch/x86/boot/main.c
index e3add85..c421af5 100644
--- a/arch/x86/boot/main.c
+++ b/arch/x86/boot/main.c
@@ -33,7 +33,7 @@ static void copy_boot_params(void)
 		u16 cl_offset;
 	};
 	const struct old_cmdline * const oldcmd =
-		(const struct old_cmdline *)OLD_CL_ADDRESS;
+		absolute_pointer(OLD_CL_ADDRESS);
 
 	BUILD_BUG_ON(sizeof(boot_params) != 4096);
 	memcpy(&boot_params.hdr, &hdr, sizeof(hdr));

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-19 10:58 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-27 19:59 [PATCH] x86/boot: Wrap literal addresses in absolute_pointer() Kees Cook
2022-02-27 20:25 ` Guenter Roeck
2022-05-19  5:41 ` Kees Cook
2022-05-19 10:57 ` [tip: x86/build] " tip-bot2 for Kees Cook

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.