| Message ID | 20220202003033.704951-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fortify: Add Clang support | expand |
On Wed, Feb 2, 2022 at 1:30 AM Kees Cook <keescook@chromium.org> wrote: > > +/* > + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size For attributes that are not supported under all compilers, we have the "Optional" lines in the comment. From a quick look in Godbolt, `__pass_object_size__` and `__overloadable__` are supported for all Clang >= 11 but not GCC/ICC. Thus, could you please add to the comment: * Optional: not supported by gcc. * Optional: not supported by icc. to those two patches? For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I assume >= 14, thus could you please add: * Optional: only supported since clang >= 14. ? Thanks! > + * The "type" argument should match the __builtin_object_size(p, type) usage. This should go above on top of the comment (it is true there is one case that does not follow it, but that one has to be cleaned up). Also, this bit seems to be explained in the Clang documentation (i.e. not kernel-specific). Do you think we need it here? Cheers, Miguel
On Wed, Feb 2, 2022 at 2:11 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I > assume >= 14, thus could you please add: > > * Optional: only supported since clang >= 14. ...and the other two too: > * Optional: not supported by gcc. > * Optional: not supported by icc. Cheers, Miguel
On Wed, Feb 02, 2022 at 02:11:51AM +0100, Miguel Ojeda wrote: > On Wed, Feb 2, 2022 at 1:30 AM Kees Cook <keescook@chromium.org> wrote: > > > > +/* > > + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size > > For attributes that are not supported under all compilers, we have the > "Optional" lines in the comment. From a quick look in Godbolt, > `__pass_object_size__` and `__overloadable__` are supported for all > Clang >= 11 but not GCC/ICC. Thus, could you please add to the > comment: > > * Optional: not supported by gcc. > * Optional: not supported by icc. > > to those two patches? > > For `__diagnose_as_builtin__`, I only see it on Clang trunk, so I > assume >= 14, thus could you please add: > > * Optional: only supported since clang >= 14. > > ? > > Thanks! Gotcha, I will adjust these. > > + * The "type" argument should match the __builtin_object_size(p, type) usage. > > This should go above on top of the comment (it is true there is one > case that does not follow it, but that one has to be cleaned up). > > Also, this bit seems to be explained in the Clang documentation (i.e. > not kernel-specific). Do you think we need it here? It's the kind of detail I feel like I might forget a month from now, so I added it as a bit of a hint. If you think it's too redundant, I can leave it off.
On Wed, Feb 2, 2022 at 10:09 PM Kees Cook <keescook@chromium.org> wrote: > > It's the kind of detail I feel like I might forget a month from now, so > I added it as a bit of a hint. If you think it's too redundant, I can > leave it off. If you think it is valuable having it nearby (e.g. when inspecting the definition of the attribute in a call site), then it is fine having them -- I am all for documentation (and we could consider having a small summary/one-liner for all attributes). But, in general, I would avoid repeating all the compiler docs (or, if needed, we could fix those upstream if they are not clear etc.). Thanks! Cheers, Miguel
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h index 37e260020221..cc751e0770f5 100644 --- a/include/linux/compiler_attributes.h +++ b/include/linux/compiler_attributes.h @@ -263,6 +263,17 @@ */ #define __packed __attribute__((__packed__)) +/* + * clang: https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size + * + * The "type" argument should match the __builtin_object_size(p, type) usage. + */ +#if __has_attribute(__pass_object_size__) +# define __pass_object_size(type) __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). This 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 | 11 +++++++++++ 1 file changed, 11 insertions(+)