| Message ID | 20220203173307.1033257-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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 to gain greater visibility to type information when using > __builtin_object_size(), Clang has a function attribute "pass_object_size" > that will make size information available for marked arguments in > a function by way of implicit additional function arguments that are > then wired up the __builtin_object_size(). > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround > to Clang's __builtin_object_size() having limited visibility[1] into types > across function calls (even inlines). > > Since any usage must also be const, include it in the macro. I really don't like hiding the qualifier in the argument-attribute macro like that. I'd rather we be more explicit about changing the function interfaces in include/linux/fortify-string.h. For instance, in patch 4/4, let's take a look at this hunk: -__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) ... +__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3) +char *strncpy(char * POS p, const char *q, __kernel_size_t size) manually expanded, this has changed the qualifiers on the type of the first parameter from `char *` to `char * const`. I think it might be helpful to quote the docs (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size) >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition. One thing that's concerning to me is though: >> It is an error to take the address of a function with pass_object_size on any of its parameters. Surely the kernel has indirect calls to some of these functions somewhere? Is that just an issue for C++ name-mangling perhaps? > > This attribute has an additional benefit that it can be used even on > non-inline functions to gain argument size information. > > [1] https://github.com/llvm/llvm-project/issues/53516 > > 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 | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h > index 37e260020221..4ce370094e3a 100644 > --- a/include/linux/compiler_attributes.h > +++ b/include/linux/compiler_attributes.h > @@ -263,6 +263,20 @@ > */ > #define __packed __attribute__((__packed__)) > > +/* > + * Note: the "type" argument should match any __builtin_object_size(p, type) usage. > + * > + * Optional: not supported by gcc. > + * Optional: not supported by icc. > + * > + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size > + */ > +#if __has_attribute(__pass_object_size__) > +# define __pass_object_size(type) const __attribute__((__pass_object_size__(type))) > +#else > +# define __pass_object_size(type) > +#endif > + > /* > * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute > */ > -- > 2.30.2 >
On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote: > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > In order to gain greater visibility to type information when using > > __builtin_object_size(), Clang has a function attribute "pass_object_size" > > that will make size information available for marked arguments in > > a function by way of implicit additional function arguments that are > > then wired up the __builtin_object_size(). > > > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround > > to Clang's __builtin_object_size() having limited visibility[1] into types > > across function calls (even inlines). > > > > Since any usage must also be const, include it in the macro. > > I really don't like hiding the qualifier in the argument-attribute > macro like that. I'd rather we be more explicit about changing the > function interfaces in include/linux/fortify-string.h. It seemed redundant to have it separate when it would always be needed. In v5 I had the const in the BOS (now POS) macro, but realized that this was silly since it would always need to be that way for __pos. > For instance, in patch 4/4, let's take a look at this hunk: > > -__FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) > ... > +__FORTIFY_INLINE __diagnose_as(__builtin_strncpy, 1, 2, 3) > +char *strncpy(char * POS p, const char *q, __kernel_size_t size) > > manually expanded, this has changed the qualifiers on the type of the > first parameter from `char *` to `char * const`. Yup, that's expected, and I wanted to tie it strictly to the expansion of __pass_object_size since otherwise GCC would gain the "const" bit (which technically shouldn't matter, but this whole series has been nothing but corner case after corner case, and I didn't want to risk another one). > I think it might be helpful to quote the docs > (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size) > > >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition. > > One thing that's concerning to me is though: > > >> It is an error to take the address of a function with pass_object_size on any of its parameters. > > Surely the kernel has indirect calls to some of these functions > somewhere? Is that just an issue for C++ name-mangling perhaps? AFAIU, this shouldn't be a problem for any of these. Nothing is trying to take memcpy, memset, etc by address. The macro-ified version of this change proved that out. :)
On Thu, Feb 3, 2022 at 12:58 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote: > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > In order to gain greater visibility to type information when using > > > __builtin_object_size(), Clang has a function attribute "pass_object_size" > > > that will make size information available for marked arguments in > > > a function by way of implicit additional function arguments that are > > > then wired up the __builtin_object_size(). > > > > > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround > > > to Clang's __builtin_object_size() having limited visibility[1] into types > > > across function calls (even inlines). > > > > > > Since any usage must also be const, include it in the macro. > > > > I think it might be helpful to quote the docs > > (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size) > > > > >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition. > > > > One thing that's concerning to me is though: > > > > >> It is an error to take the address of a function with pass_object_size on any of its parameters. > > > > Surely the kernel has indirect calls to some of these functions > > somewhere? Is that just an issue for C++ name-mangling perhaps? > > AFAIU, this shouldn't be a problem for any of these. Nothing is trying > to take memcpy, memset, etc by address. The macro-ified version of this > change proved that out. :) I thought Sami had found a location where memcpy was invoked indirectly as part of his kcfi work? Maybe I'm misremembering. https://github.com/samitolvanen/linux/commit/46a777fb35784a8c6daf13d67de8bfb5148adc2a#diff-a27660992abdf360d01deac6364db31836d0d98b5d9573b7fc10a6785a669975R16
On Thu, Feb 03, 2022 at 02:01:06PM -0800, Nick Desaulniers wrote: > On Thu, Feb 3, 2022 at 12:58 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Thu, Feb 03, 2022 at 12:18:24PM -0800, Nick Desaulniers wrote: > > > On Thu, Feb 3, 2022 at 9:33 AM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > In order to gain greater visibility to type information when using > > > > __builtin_object_size(), Clang has a function attribute "pass_object_size" > > > > that will make size information available for marked arguments in > > > > a function by way of implicit additional function arguments that are > > > > then wired up the __builtin_object_size(). > > > > > > > > This is needed to implement FORTIFY_SOURCE in Clang, as a workaround > > > > to Clang's __builtin_object_size() having limited visibility[1] into types > > > > across function calls (even inlines). > > > > > > > > Since any usage must also be const, include it in the macro. > > > > > > I think it might be helpful to quote the docs > > > (https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size) > > > > > > >> Additionally, any parameter that pass_object_size is applied to must be marked const at its function’s definition. > > > > > > One thing that's concerning to me is though: > > > > > > >> It is an error to take the address of a function with pass_object_size on any of its parameters. > > > > > > Surely the kernel has indirect calls to some of these functions > > > somewhere? Is that just an issue for C++ name-mangling perhaps? > > > > AFAIU, this shouldn't be a problem for any of these. Nothing is trying > > to take memcpy, memset, etc by address. The macro-ified version of this > > change proved that out. :) > > I thought Sami had found a location where memcpy was invoked > indirectly as part of his kcfi work? Maybe I'm misremembering. > > https://github.com/samitolvanen/linux/commit/46a777fb35784a8c6daf13d67de8bfb5148adc2a#diff-a27660992abdf360d01deac6364db31836d0d98b5d9573b7fc10a6785a669975R16 Hm, I've had memcpy as a macro for a while now, so dunno! That's not a sensible thing to call indirectly. :)
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h index 37e260020221..4ce370094e3a 100644 --- a/include/linux/compiler_attributes.h +++ b/include/linux/compiler_attributes.h @@ -263,6 +263,20 @@ */ #define __packed __attribute__((__packed__)) +/* + * Note: the "type" argument should match any __builtin_object_size(p, type) usage. + * + * Optional: not supported by gcc. + * Optional: not supported by icc. + * + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size + */ +#if __has_attribute(__pass_object_size__) +# define __pass_object_size(type) const __attribute__((__pass_object_size__(type))) +#else +# define __pass_object_size(type) +#endif + /* * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-pure-function-attribute */
In order to gain greater visibility to type information when using __builtin_object_size(), Clang has a function attribute "pass_object_size" that will make size information available for marked arguments in a function by way of implicit additional function arguments that are then wired up the __builtin_object_size(). This is needed to implement FORTIFY_SOURCE in Clang, as a workaround to Clang's __builtin_object_size() having limited visibility[1] into types across function calls (even inlines). Since any usage must also be const, include it in the macro. This attribute has an additional benefit that it can be used even on non-inline functions to gain argument size information. [1] https://github.com/llvm/llvm-project/issues/53516 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 | 14 ++++++++++++++ 1 file changed, 14 insertions(+)