[PATCH] Compiler Attributes: Introduce __access_*() function attribute

[PATCH] Compiler Attributes: Introduce __access_*() function attribute

[Kees Cook]

Added in GCC 10.1, the "access" function attribute to mark pointer
arguments for how they are expected to be accessed in a given function.
Both their access type (read/write, read-only, or write-only) and bounds
are specified. While it is legal to provide only the pointer argument
position and access type, design the kernel macros to require also the
bounds (element count) argument position: if a function has no bounds
argument, refactor the code to include one.

These can be used multiple times. For example:

__access_wo(2, 3) __access_ro(4, 5)
int copy_something(struct context *ctx, u32 *dst, size_t dst_count,
		   u8 *src, int src_len);

(And if "dst" will also be read, it could use __access_rw(2, 3) instead.)

These can inform the compile-time diagnostics of GCC including
-Warray-bounds, -Wstringop-overflow, etc, and can affect
__builtin_dynamic_object_size() results.

Cc: Miguel Ojeda <ojeda@kernel.org>
Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>
Cc: Nathan Chancellor <nathan@kernel.org>
Cc: Tom Rix <trix@redhat.com>
Cc: llvm@lists.linux.dev
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/compiler_attributes.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index 9a9907fad6fd..6f3d40f7ee5e 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -20,6 +20,22 @@
  * Provide links to the documentation of each supported compiler, if it exists.
  */
 
+/*
+ * Optional: only supported since gcc >= 10
+ * Optional: not supported by Clang
+ *
+ *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-access-function-attribute
+ */
+#if __has_attribute(__access__)
+#define __access_rw(ptr, count)	__attribute__((__access__(read_write, ptr, count)))
+#define __access_ro(ptr, count)	__attribute__((__access__(read_only,  ptr, count)))
+#define __access_wo(ptr, count)	__attribute__((__access__(write_only, ptr, count)))
+#else
+#define __access_rw(ptr, count)
+#define __access_ro(ptr, count)
+#define __access_wo(ptr, count)
+#endif
+
 /*
  *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alias-function-attribute
  */
-- 
2.34.1

Re: [PATCH] Compiler Attributes: Introduce __access_*() function attribute

[Gustavo A. R. Silva]

On Fri, Sep 23, 2022 at 04:54:24PM -0700, Kees Cook wrote:
> Added in GCC 10.1, the "access" function attribute to mark pointer
> arguments for how they are expected to be accessed in a given function.
> Both their access type (read/write, read-only, or write-only) and bounds
> are specified. While it is legal to provide only the pointer argument
> position and access type, design the kernel macros to require also the
> bounds (element count) argument position: if a function has no bounds
> argument, refactor the code to include one.
> 
> These can be used multiple times. For example:
> 
> __access_wo(2, 3) __access_ro(4, 5)
> int copy_something(struct context *ctx, u32 *dst, size_t dst_count,
> 		   u8 *src, int src_len);
> 
> (And if "dst" will also be read, it could use __access_rw(2, 3) instead.)
> 
> These can inform the compile-time diagnostics of GCC including
> -Warray-bounds, -Wstringop-overflow, etc, and can affect
> __builtin_dynamic_object_size() results.
> 
> Cc: Miguel Ojeda <ojeda@kernel.org>
> Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
> Cc: Nick Desaulniers <ndesaulniers@google.com>
> Cc: Nathan Chancellor <nathan@kernel.org>
> Cc: Tom Rix <trix@redhat.com>
> Cc: llvm@lists.linux.dev
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>

--
Gustavo

> ---
>  include/linux/compiler_attributes.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> index 9a9907fad6fd..6f3d40f7ee5e 100644
> --- a/include/linux/compiler_attributes.h
> +++ b/include/linux/compiler_attributes.h
> @@ -20,6 +20,22 @@
>   * Provide links to the documentation of each supported compiler, if it exists.
>   */
>  
> +/*
> + * Optional: only supported since gcc >= 10
> + * Optional: not supported by Clang
> + *
> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-access-function-attribute
> + */
> +#if __has_attribute(__access__)
> +#define __access_rw(ptr, count)	__attribute__((__access__(read_write, ptr, count)))
> +#define __access_ro(ptr, count)	__attribute__((__access__(read_only,  ptr, count)))
> +#define __access_wo(ptr, count)	__attribute__((__access__(write_only, ptr, count)))
> +#else
> +#define __access_rw(ptr, count)
> +#define __access_ro(ptr, count)
> +#define __access_wo(ptr, count)
> +#endif
> +
>  /*
>   *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alias-function-attribute
>   */
> -- 
> 2.34.1
> 

Re: [PATCH] Compiler Attributes: Introduce __access_*() function attribute

[Miguel Ojeda]

On Sat, Sep 24, 2022 at 1:54 AM Kees Cook <keescook@chromium.org> wrote:
>
> are specified. While it is legal to provide only the pointer argument
> position and access type, design the kernel macros to require also the
> bounds (element count) argument position: if a function has no bounds
> argument, refactor the code to include one.

Should this bit be included in the comment of the attribute? (since it
is specific to the kernel)

> These can be used multiple times. For example:
>
> __access_wo(2, 3) __access_ro(4, 5)
> int copy_something(struct context *ctx, u32 *dst, size_t dst_count,
>                    u8 *src, int src_len);
>
> (And if "dst" will also be read, it could use __access_rw(2, 3) instead.)

Also maybe the example could be nice there too, since it uses the
syntax for the kernel and you took the time to write it :)

By the way, shouldn't `src` typically be `const u8 *`? Given it is an
example, I would qualify it.

> +#if __has_attribute(__access__)
> +#define __access_rw(ptr, count)        __attribute__((__access__(read_write, ptr, count)))
> +#define __access_ro(ptr, count)        __attribute__((__access__(read_only,  ptr, count)))
> +#define __access_wo(ptr, count)        __attribute__((__access__(write_only, ptr, count)))
> +#else
> +#define __access_rw(ptr, count)
> +#define __access_ro(ptr, count)
> +#define __access_wo(ptr, count)
> +#endif

If you do a v2 for the above, please take the chance to indent with a
single space after the `#` (like `# define`) for consistency.

Thanks!

Cheers,
Miguel

Re: [PATCH] Compiler Attributes: Introduce __access_*() function attribute

[Kees Cook]

On Sat, Sep 24, 2022 at 12:06:31PM +0200, Miguel Ojeda wrote:
> On Sat, Sep 24, 2022 at 1:54 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > are specified. While it is legal to provide only the pointer argument
> > position and access type, design the kernel macros to require also the
> > bounds (element count) argument position: if a function has no bounds
> > argument, refactor the code to include one.
> 
> Should this bit be included in the comment of the attribute? (since it
> is specific to the kernel)

Sure; good idea!

> 
> > These can be used multiple times. For example:
> >
> > __access_wo(2, 3) __access_ro(4, 5)
> > int copy_something(struct context *ctx, u32 *dst, size_t dst_count,
> >                    u8 *src, int src_len);
> >
> > (And if "dst" will also be read, it could use __access_rw(2, 3) instead.)
> 
> Also maybe the example could be nice there too, since it uses the
> syntax for the kernel and you took the time to write it :)
> 
> By the way, shouldn't `src` typically be `const u8 *`? Given it is an
> example, I would qualify it.

Yeah, I will update this.

> 
> > +#if __has_attribute(__access__)
> > +#define __access_rw(ptr, count)        __attribute__((__access__(read_write, ptr, count)))
> > +#define __access_ro(ptr, count)        __attribute__((__access__(read_only,  ptr, count)))
> > +#define __access_wo(ptr, count)        __attribute__((__access__(write_only, ptr, count)))
> > +#else
> > +#define __access_rw(ptr, count)
> > +#define __access_ro(ptr, count)
> > +#define __access_wo(ptr, count)
> > +#endif
> 
> If you do a v2 for the above, please take the chance to indent with a
> single space after the `#` (like `# define`) for consistency.

Fixed! I will send a v2.

-- 
Kees Cook