[PATCH mips-fixes] MIPS: fix fortify panic when copying asm exception handlers

[PATCH mips-fixes] MIPS: fix fortify panic when copying asm exception handlers

[Alexander Lobakin]

With KCFLAGS="-O3", I was able to trigger a fortify-source
memcpy() overflow panic on set_vi_srs_handler().
Although O3 level is not supported in the mainline, under some
conditions that may've happened with any optimization settings,
it's just a matter of inlining luck. The panic itself is correct,
more precisely, 50/50 false-positive and not at the same time.
From the one side, no real overflow happens. Exception handler
defined in asm just gets copied to some reserved places in the
memory.
But the reason behind is that C code refers to that exception
handler declares it as `char`, i.e. something of 1 byte length.
It's obvious that the asm function itself is way more than 1 byte,
so fortify logics thought we are going to past the symbol declared.
The standard way to refer to asm symbols from C code which is not
supposed to be called from C is to declare them as
`extern const u8[]`. This is fully correct from any point of view,
as any code itself is just a bunch of bytes (including 0 as it is
for syms like _stext/_etext/etc.), and the exact size is not known
at the moment of compilation.
Adjust the type of the except_vec_vi_*() and related variables.
Make set_handler() take `const` as a second argument to avoid
cast-away warnings and give a little more room for optimization.

Fixes: e01402b115cc ("More AP / SP bits for the 34K, the Malta bits and things. Still wants")
Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
Cc: stable@vger.kernel.org # 3.10+
Signed-off-by: Alexander Lobakin <alobakin@pm.me>
---
 arch/mips/include/asm/setup.h |  2 +-
 arch/mips/kernel/traps.c      | 22 +++++++++++-----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/mips/include/asm/setup.h b/arch/mips/include/asm/setup.h
index bb36a400203d..8c56b862fd9c 100644
--- a/arch/mips/include/asm/setup.h
+++ b/arch/mips/include/asm/setup.h
@@ -16,7 +16,7 @@ static inline void setup_8250_early_printk_port(unsigned long base,
 	unsigned int reg_shift, unsigned int timeout) {}
 #endif

-extern void set_handler(unsigned long offset, void *addr, unsigned long len);
+void set_handler(unsigned long offset, const void *addr, unsigned long len);
 extern void set_uncached_handler(unsigned long offset, void *addr, unsigned long len);

 typedef void (*vi_handler_t)(void);
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index a486486b2355..246c6a6b0261 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -2091,19 +2091,19 @@ static void *set_vi_srs_handler(int n, vi_handler_t addr, int srs)
 		 * If no shadow set is selected then use the default handler
 		 * that does normal register saving and standard interrupt exit
 		 */
-		extern char except_vec_vi, except_vec_vi_lui;
-		extern char except_vec_vi_ori, except_vec_vi_end;
-		extern char rollback_except_vec_vi;
-		char *vec_start = using_rollback_handler() ?
-			&rollback_except_vec_vi : &except_vec_vi;
+		extern const u8 except_vec_vi[], except_vec_vi_lui[];
+		extern const u8 except_vec_vi_ori[], except_vec_vi_end[];
+		extern const u8 rollback_except_vec_vi[];
+		const u8 *vec_start = using_rollback_handler() ?
+				      rollback_except_vec_vi : except_vec_vi;
 #if defined(CONFIG_CPU_MICROMIPS) || defined(CONFIG_CPU_BIG_ENDIAN)
-		const int lui_offset = &except_vec_vi_lui - vec_start + 2;
-		const int ori_offset = &except_vec_vi_ori - vec_start + 2;
+		const int lui_offset = except_vec_vi_lui - vec_start + 2;
+		const int ori_offset = except_vec_vi_ori - vec_start + 2;
 #else
-		const int lui_offset = &except_vec_vi_lui - vec_start;
-		const int ori_offset = &except_vec_vi_ori - vec_start;
+		const int lui_offset = except_vec_vi_lui - vec_start;
+		const int ori_offset = except_vec_vi_ori - vec_start;
 #endif
-		const int handler_len = &except_vec_vi_end - vec_start;
+		const int handler_len = except_vec_vi_end - vec_start;

 		if (handler_len > VECTORSPACING) {
 			/*
@@ -2311,7 +2311,7 @@ void per_cpu_trap_init(bool is_boot_cpu)
 }

 /* Install CPU exception handler */
-void set_handler(unsigned long offset, void *addr, unsigned long size)
+void set_handler(unsigned long offset, const void *addr, unsigned long size)
 {
 #ifdef CONFIG_CPU_MICROMIPS
 	memcpy((void *)(ebase + offset), ((unsigned char *)addr - 1), size);
--
2.35.1

Re: [PATCH mips-fixes] MIPS: fix fortify panic when copying asm exception handlers

[Thomas Bogendoerfer]

On Wed, Feb 23, 2022 at 01:30:23AM +0000, Alexander Lobakin wrote:
> With KCFLAGS="-O3", I was able to trigger a fortify-source
> memcpy() overflow panic on set_vi_srs_handler().
> Although O3 level is not supported in the mainline, under some
> conditions that may've happened with any optimization settings,
> it's just a matter of inlining luck. The panic itself is correct,
> more precisely, 50/50 false-positive and not at the same time.
> >From the one side, no real overflow happens. Exception handler
> defined in asm just gets copied to some reserved places in the
> memory.
> But the reason behind is that C code refers to that exception
> handler declares it as `char`, i.e. something of 1 byte length.
> It's obvious that the asm function itself is way more than 1 byte,
> so fortify logics thought we are going to past the symbol declared.
> The standard way to refer to asm symbols from C code which is not
> supposed to be called from C is to declare them as
> `extern const u8[]`. This is fully correct from any point of view,
> as any code itself is just a bunch of bytes (including 0 as it is
> for syms like _stext/_etext/etc.), and the exact size is not known
> at the moment of compilation.
> Adjust the type of the except_vec_vi_*() and related variables.
> Make set_handler() take `const` as a second argument to avoid
> cast-away warnings and give a little more room for optimization.
> 
> Fixes: e01402b115cc ("More AP / SP bits for the 34K, the Malta bits and things. Still wants")
> Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
> Cc: stable@vger.kernel.org # 3.10+

I like your patch, but I have a problem with these tags. If I understand
your description correctly there is no bug, but because of the way the
code is written fortify-source gets confused. So if it doesn't fix
anything, there shouldn't be Fixes tags, IMHO. If you agree, I'll
apply this patch to mips-next and remove the tags.

Thomas.

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

Re: [PATCH mips-fixes] MIPS: fix fortify panic when copying asm exception handlers

[Alexander Lobakin]

From: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Date: Tue, 1 Mar 2022 17:34:11 +0100

> On Wed, Feb 23, 2022 at 01:30:23AM +0000, Alexander Lobakin wrote:
> > With KCFLAGS="-O3", I was able to trigger a fortify-source
> > memcpy() overflow panic on set_vi_srs_handler().
> > Although O3 level is not supported in the mainline, under some
> > conditions that may've happened with any optimization settings,
> > it's just a matter of inlining luck. The panic itself is correct,
> > more precisely, 50/50 false-positive and not at the same time.
> > >From the one side, no real overflow happens. Exception handler
> > defined in asm just gets copied to some reserved places in the
> > memory.
> > But the reason behind is that C code refers to that exception
> > handler declares it as `char`, i.e. something of 1 byte length.
> > It's obvious that the asm function itself is way more than 1 byte,
> > so fortify logics thought we are going to past the symbol declared.
> > The standard way to refer to asm symbols from C code which is not
> > supposed to be called from C is to declare them as
> > `extern const u8[]`. This is fully correct from any point of view,
> > as any code itself is just a bunch of bytes (including 0 as it is
> > for syms like _stext/_etext/etc.), and the exact size is not known
> > at the moment of compilation.
> > Adjust the type of the except_vec_vi_*() and related variables.
> > Make set_handler() take `const` as a second argument to avoid
> > cast-away warnings and give a little more room for optimization.
> >
> > Fixes: e01402b115cc ("More AP / SP bits for the 34K, the Malta bits and things. Still wants")
> > Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
> > Cc: stable@vger.kernel.org # 3.10+
>
> I like your patch, but I have a problem with these tags. If I understand
> your description correctly there is no bug, but because of the way the
> code is written fortify-source gets confused. So if it doesn't fix
> anything, there shouldn't be Fixes tags, IMHO. If you agree, I'll
> apply this patch to mips-next and remove the tags.

Oh, sorry for the late reply.
Sure, feel free to apply to mips-next. Yeah, there is no real bug,
just not-really-100%-clean-code which works anyways.

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

Thanks,
Al

Re: [PATCH mips-fixes] MIPS: fix fortify panic when copying asm exception handlers

[Thomas Bogendoerfer]

On Wed, Feb 23, 2022 at 01:30:23AM +0000, Alexander Lobakin wrote:
> With KCFLAGS="-O3", I was able to trigger a fortify-source
> memcpy() overflow panic on set_vi_srs_handler().
> Although O3 level is not supported in the mainline, under some
> conditions that may've happened with any optimization settings,
> it's just a matter of inlining luck. The panic itself is correct,
> more precisely, 50/50 false-positive and not at the same time.
> >From the one side, no real overflow happens. Exception handler
> defined in asm just gets copied to some reserved places in the
> memory.
> But the reason behind is that C code refers to that exception
> handler declares it as `char`, i.e. something of 1 byte length.
> It's obvious that the asm function itself is way more than 1 byte,
> so fortify logics thought we are going to past the symbol declared.
> The standard way to refer to asm symbols from C code which is not
> supposed to be called from C is to declare them as
> `extern const u8[]`. This is fully correct from any point of view,
> as any code itself is just a bunch of bytes (including 0 as it is
> for syms like _stext/_etext/etc.), and the exact size is not known
> at the moment of compilation.
> Adjust the type of the except_vec_vi_*() and related variables.
> Make set_handler() take `const` as a second argument to avoid
> cast-away warnings and give a little more room for optimization.
> 
> Fixes: e01402b115cc ("More AP / SP bits for the 34K, the Malta bits and things. Still wants")
> Fixes: c65a5480ff29 ("[MIPS] Fix potential latency problem due to non-atomic cpu_wait.")
> Cc: stable@vger.kernel.org # 3.10+
> Signed-off-by: Alexander Lobakin <alobakin@pm.me>
> ---
>  arch/mips/include/asm/setup.h |  2 +-
>  arch/mips/kernel/traps.c      | 22 +++++++++++-----------
>  2 files changed, 12 insertions(+), 12 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 ]