Re: [PATCH] ubsan: Avoid i386 UBSAN handler crashes with Clang

[Kees Cook]

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@xxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> > ---
> > Cc: Marco Elver <elver@xxxxxxxxxx>
> > Cc: Andrey Konovalov <andreyknvl@xxxxxxxxx>
> > Cc: Andrey Ryabinin <ryabinin.a.a@xxxxxxxxx>
> > Cc: Nathan Chancellor <nathan@xxxxxxxxxx>
> > Cc: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
> > Cc: Bill Wendling <morbo@xxxxxxxxxx>
> > Cc: Justin Stitt <justinstitt@xxxxxxxxxx>
> > Cc: llvm@xxxxxxxxxxxxxxx
> > Cc: kasan-dev@xxxxxxxxxxxxxxxx
> > Cc: linux-hardening@xxxxxxxxxxxxxxx
> > ---
> >  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