diff mbox series

[1/4] Compiler Attributes: Add Clang's __pass_object_size

Message ID 20220202003033.704951-2-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series fortify: Add Clang support | expand

Commit Message

Kees Cook Feb. 2, 2022, 12:30 a.m. UTC
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(+)

Comments

Miguel Ojeda Feb. 2, 2022, 1:11 a.m. UTC | #1
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
Miguel Ojeda Feb. 2, 2022, 1:13 a.m. UTC | #2
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
Kees Cook Feb. 2, 2022, 9:09 p.m. UTC | #3
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.
Miguel Ojeda Feb. 2, 2022, 9:19 p.m. UTC | #4
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 mbox series

Patch

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
  */