Date: Wed, 24 Apr 2024 15:32:10 -0700
From: Kees Cook <keescook@...omium.org>
To: Nathan Chancellor <nathan@...nel.org>
Cc: Marco Elver <elver@...gle.com>, Andrey Konovalov <andreyknvl@...il.com>,
	Andrey Ryabinin <ryabinin.a.a@...il.com>,
	Nick Desaulniers <ndesaulniers@...gle.com>,
	Bill Wendling <morbo@...gle.com>,
	Justin Stitt <justinstitt@...gle.com>, llvm@...ts.linux.dev,
	kasan-dev@...glegroups.com, linux-hardening@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ubsan: Avoid i386 UBSAN handler crashes with Clang

On Wed, Apr 24, 2024 at 12:26:52PM -0700, Nathan Chancellor wrote:
> Hi Kees,
> 
> On Wed, Apr 24, 2024 at 09:29:43AM -0700, Kees Cook wrote:
> > When generating Runtime Calls, Clang doesn't respect the -mregparm=3
> > option used on i386. Hopefully this will be fixed correctly in Clang 19:
> > https://github.com/llvm/llvm-project/pull/89707
> > but we need to fix this for earlier Clang versions today. Force the
> > calling convention to use non-register arguments.
> > 
> > Reported-by: ernsteiswuerfel
> 
> FWIW, I think this can be
> 
>   Reported-by: Erhard Furtner <erhard_f@...lbox.org>
> 
> since it has been used in the kernel before, the reporter is well known
> :)

Ah! Okay, thanks. I wasn't able to find an associated email address. :)

> 
> > Closes: https://github.com/KSPP/linux/issues/350
> > Signed-off-by: Kees Cook <keescook@...omium.org>
> > ---
> > Cc: Marco Elver <elver@...gle.com>
> > Cc: Andrey Konovalov <andreyknvl@...il.com>
> > Cc: Andrey Ryabinin <ryabinin.a.a@...il.com>
> > Cc: Nathan Chancellor <nathan@...nel.org>
> > Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> > Cc: Bill Wendling <morbo@...gle.com>
> > Cc: Justin Stitt <justinstitt@...gle.com>
> > Cc: llvm@...ts.linux.dev
> > Cc: kasan-dev@...glegroups.com
> > Cc: linux-hardening@...r.kernel.org
> > ---
> >  lib/ubsan.h | 41 +++++++++++++++++++++++++++--------------
> >  1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/lib/ubsan.h b/lib/ubsan.h
> > index 50ef50811b7c..978828f6099d 100644
> > --- a/lib/ubsan.h
> > +++ b/lib/ubsan.h
> > @@ -124,19 +124,32 @@ typedef s64 s_max;
> >  typedef u64 u_max;
> >  #endif
> >  
> > -void __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
> > -void __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
> > -void __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
> > -void __ubsan_handle_negate_overflow(void *_data, void *old_val);
> > -void __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
> > -void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
> > -void __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
> > -void __ubsan_handle_out_of_bounds(void *_data, void *index);
> > -void __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
> > -void __ubsan_handle_builtin_unreachable(void *_data);
> > -void __ubsan_handle_load_invalid_value(void *_data, void *val);
> > -void __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
> > -					 unsigned long align,
> > -					 unsigned long offset);
> > +/*
> > + * When generating Runtime Calls, Clang doesn't respect the -mregparm=3
> > + * option used on i386. Hopefully this will be fixed correctly in Clang 19:
> > + * https://github.com/llvm/llvm-project/pull/89707
> > + * but we need to fix this for earlier Clang versions today. Force the
> 
> It may be better to link to the tracking issue upstream instead of the
> pull request just in case someone comes up with an alternative fix (not
> that I think your change is wrong or anything but it seems like that
> happens every so often).
> 
> I also get leary of the version information in the comment, even though
> I don't doubt this will be fixed in clang 19.
> 
> > + * calling convention to use non-register arguments.
> > + */
> > +#if defined(__clang__) && defined(CONFIG_X86_32)
> 
> While __clang__ is what causes CONFIG_CC_IS_CLANG to get set and there
> is some existing use of it throughout the kernel, I think
> CONFIG_CC_IS_CLANG makes it easier to audit the workarounds that we
> have, plus this will be presumably covered to
> 
>   CONFIG_CLANG_VERSION < 190000

Yeah, that seems much cleaner. I will adjust it...

> 
> when the fix actually lands. This file is not expected to be used
> outside of the kernel, right? That is the only thing I could think of
> where this distinction would actually matter.
> 
> > +# define ubsan_linkage asmlinkage
> 
> Heh, clever...
> 
> > +#else
> > +# define ubsan_linkage /**/
> 
> Why is this defined as a comment rather than just nothing?

I dunno; this is a coding style glitch of mine. :P I will drop it.

Thanks for the review!

-Kees

> 
> > +#endif
> > +
> > +void ubsan_linkage __ubsan_handle_add_overflow(void *data, void *lhs, void *rhs);
> > +void ubsan_linkage __ubsan_handle_sub_overflow(void *data, void *lhs, void *rhs);
> > +void ubsan_linkage __ubsan_handle_mul_overflow(void *data, void *lhs, void *rhs);
> > +void ubsan_linkage __ubsan_handle_negate_overflow(void *_data, void *old_val);
> > +void ubsan_linkage __ubsan_handle_divrem_overflow(void *_data, void *lhs, void *rhs);
> > +void ubsan_linkage __ubsan_handle_type_mismatch(struct type_mismatch_data *data, void *ptr);
> > +void ubsan_linkage __ubsan_handle_type_mismatch_v1(void *_data, void *ptr);
> > +void ubsan_linkage __ubsan_handle_out_of_bounds(void *_data, void *index);
> > +void ubsan_linkage __ubsan_handle_shift_out_of_bounds(void *_data, void *lhs, void *rhs);
> > +void ubsan_linkage __ubsan_handle_builtin_unreachable(void *_data);
> > +void ubsan_linkage __ubsan_handle_load_invalid_value(void *_data, void *val);
> > +void ubsan_linkage __ubsan_handle_alignment_assumption(void *_data, unsigned long ptr,
> > +						       unsigned long align,
> > +						       unsigned long offset);
> >  
> >  #endif
> > -- 
> > 2.34.1
> > 
> > 

-- 
Kees Cook

Powered by blists - more mailing lists