| Message ID | 20220203173307.1033257-3-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | a382dbba4f1e3e7ba1433bc2f33f661491572a26 |
| Headers | show |
| Series | fortify: Add Clang support | expand |
On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > In order for FORTIFY_SOURCE to use __pass_object_size on an "inline > extern" function, as all the fortified string functions are, the functions s/inline extern/extern inline/ Yes, they're the same thing, but it sounds backwards when I read it. > must be marked as being overloadable (i.e. different prototypes). > This allows the __pass_object_size versions to take precedence. Is this because of the `const` additions to the function signatures?
On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote: > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > In order for FORTIFY_SOURCE to use __pass_object_size on an "inline > > extern" function, as all the fortified string functions are, the functions > > s/inline extern/extern inline/ I will update that. :) > > Yes, they're the same thing, but it sounds backwards when I read it. > > > must be marked as being overloadable (i.e. different prototypes). > > This allows the __pass_object_size versions to take precedence. > > Is this because of the `const` additions to the function signatures? That might be an issue, but the *real* issue is the implicit mutation of the function into an inline with _additional_ arguments. i.e. char *strcpy(char * POS p, const char * POS q) is really char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q) (i.e. what I was doing with macros, but all internally and still an extern inline)
On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote: > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > must be marked as being overloadable (i.e. different prototypes). > > > This allows the __pass_object_size versions to take precedence. > > > > Is this because of the `const` additions to the function signatures? > > That might be an issue, but the *real* issue is the implicit mutation of > the function into an inline with _additional_ arguments. i.e. > > char *strcpy(char * POS p, const char * POS q) > > is really > > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q) > > (i.e. what I was doing with macros, but all internally and still an > extern inline) What do you mean "is really"? 4/4 doesn't change the number of parameters in strcpy explicitly in the definition AFAICT.
On Thu, Feb 03, 2022 at 02:11:48PM -0800, Nick Desaulniers wrote: > On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote: > > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > must be marked as being overloadable (i.e. different prototypes). > > > > This allows the __pass_object_size versions to take precedence. > > > > > > Is this because of the `const` additions to the function signatures? > > > > That might be an issue, but the *real* issue is the implicit mutation of > > the function into an inline with _additional_ arguments. i.e. > > > > char *strcpy(char * POS p, const char * POS q) > > > > is really > > > > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q) > > > > (i.e. what I was doing with macros, but all internally and still an > > extern inline) > > What do you mean "is really"? 4/4 doesn't change the number of > parameters in strcpy explicitly in the definition AFAICT. It really does change the number of parameters. See the IR difference: $ cat example.c #ifdef USE_POS # define POS __attribute__((pass_object_size(1))) #else # define POS #endif int func(void * const POS); struct foo { int a; char *b; }; void usage(struct foo *example) { func(example); } $ IR="-O2 -Xclang -disable-llvm-passes -emit-llvm -S" $ clang example.c $IR -o normal.ll $ clang -DUSE_POS example.c $IR -o pos.ll $ diff -u normal.ll pos.ll --- normal.ll 2022-02-03 16:23:39.734065036 -0800 +++ pos.ll 2022-02-03 16:23:49.518083451 -0800 @@ -11,14 +11,19 @@ store %struct.foo* %0, %struct.foo** %2, align 8, !tbaa !3 %3 = load %struct.foo*, %struct.foo** %2, align 8, !tbaa !3 %4 = bitcast %struct.foo* %3 to i8* - %5 = call i32 @func(i8* noundef %4) + %5 = call i64 @llvm.objectsize.i64.p0i8(i8* %4, i1 false, i1 true, i1 false) + %6 = call i32 @func(i8* noundef %4, i64 noundef %5) ret void } ... This is basically doing internally exactly what I was doing in v4 and earlier with macros (passing in the caller's view of __bos(arg, 1)).
On Thu, Feb 3, 2022 at 4:26 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Feb 03, 2022 at 02:11:48PM -0800, Nick Desaulniers wrote: > > On Thu, Feb 3, 2022 at 1:04 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > On Thu, Feb 03, 2022 at 12:26:15PM -0800, Nick Desaulniers wrote: > > > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > > > must be marked as being overloadable (i.e. different prototypes). > > > > > This allows the __pass_object_size versions to take precedence. > > > > > > > > Is this because of the `const` additions to the function signatures? > > > > > > That might be an issue, but the *real* issue is the implicit mutation of > > > the function into an inline with _additional_ arguments. i.e. > > > > > > char *strcpy(char * POS p, const char * POS q) > > > > > > is really > > > > > > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q) > > > > > > (i.e. what I was doing with macros, but all internally and still an > > > extern inline) > > > > What do you mean "is really"? 4/4 doesn't change the number of > > parameters in strcpy explicitly in the definition AFAICT. > > It really does change the number of parameters. See the IR difference: > > $ cat example.c > #ifdef USE_POS > # define POS __attribute__((pass_object_size(1))) > #else > # define POS > #endif > > int func(void * const POS); > > struct foo > { > int a; > char *b; > }; > > void usage(struct foo *example) > { > func(example); > } > > $ IR="-O2 -Xclang -disable-llvm-passes -emit-llvm -S" > $ clang example.c $IR -o normal.ll > $ clang -DUSE_POS example.c $IR -o pos.ll > $ diff -u normal.ll pos.ll > --- normal.ll 2022-02-03 16:23:39.734065036 -0800 > +++ pos.ll 2022-02-03 16:23:49.518083451 -0800 > @@ -11,14 +11,19 @@ > store %struct.foo* %0, %struct.foo** %2, align 8, !tbaa !3 > %3 = load %struct.foo*, %struct.foo** %2, align 8, !tbaa !3 > %4 = bitcast %struct.foo* %3 to i8* > - %5 = call i32 @func(i8* noundef %4) > + %5 = call i64 @llvm.objectsize.i64.p0i8(i8* %4, i1 false, i1 true, i1 false) > + %6 = call i32 @func(i8* noundef %4, i64 noundef %5) > ret void > } > ... Woah, that's fancy. > > This is basically doing internally exactly what I was doing in v4 and > earlier with macros (passing in the caller's view of __bos(arg, 1)). Yeah. Ok now it all makes sense to me. With the s/inline extern/extern inline/: Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
On Thu, Feb 3, 2022 at 10:04 PM Kees Cook <keescook@chromium.org> wrote: > > That might be an issue, but the *real* issue is the implicit mutation of > the function into an inline with _additional_ arguments. i.e. > > char *strcpy(char * POS p, const char * POS q) > > is really > > char *strcpy(char * const p, const char * const q, size_t __size_of_p, size_t __size_of_q) Shouldn't that be char *strcpy(char * const p, size_t __size_of_p, const char * const q, size_t __size_of_q) ? i.e. the docs point at this but say: "...and implicitly pass the result of this call in as an invisible argument of type `size_t` directly after the parameter annotated with `pass_object_size`." Cheers, Miguel
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h index 4ce370094e3a..dc3bf2a6e1c9 100644 --- a/include/linux/compiler_attributes.h +++ b/include/linux/compiler_attributes.h @@ -257,6 +257,18 @@ */ #define __noreturn __attribute__((__noreturn__)) +/* + * Optional: not supported by gcc. + * Optional: not supported by icc. + * + * clang: https://clang.llvm.org/docs/AttributeReference.html#overloadable + */ +#if __has_attribute(__overloadable__) +# define __overloadable __attribute__((__overloadable__)) +#else +# define __overloadable +#endif + /* * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute * clang: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-packed-variable-attribute
In order for FORTIFY_SOURCE to use __pass_object_size on an "inline extern" function, as all the fortified string functions are, the functions must be marked as being overloadable (i.e. different prototypes). This allows the __pass_object_size versions to take precedence. Cc: Miguel Ojeda <ojeda@kernel.org> Cc: Nick Desaulniers <ndesaulniers@google.com> Cc: Nathan Chancellor <nathan@kernel.org> Cc: llvm@lists.linux.dev Signed-off-by: Kees Cook <keescook@chromium.org> --- include/linux/compiler_attributes.h | 12 ++++++++++++ 1 file changed, 12 insertions(+)