[0/5] Add __alloc_size() for better bounds checking

Message

Kees Cook Aug. 18, 2021, 5:08 a.m. UTC

Hi,

GCC and Clang both use the "alloc_size" attribute to assist with bounds
checking around the use of allocation functions. Add the attribute,
adjust the Makefile to silence needless warnings, and add the hints to
the allocators where possible. These changes have been in use for a
while now in GrapheneOS.

To build without warnings, this series needs a couple small fixes for
allmodconfig, which I sent separately:
https://lore.kernel.org/lkml/[e-mail]/
https://lore.kernel.org/lkml/[e-mail]/
https://lore.kernel.org/lkml/[e-mail]/

I figure I can take this via my "overflow" series, or it could go via
-mm?

-Kees

Kees Cook (5):
  Compiler Attributes: Add __alloc_size() for better bounds checking
  slab: Add __alloc_size attributes for better bounds checking
  mm/page_alloc: Add __alloc_size attributes for better bounds checking
  percpu: Add __alloc_size attributes for better bounds checking
  mm/vmalloc: Add __alloc_size attributes for better bounds checking

 Makefile                            |  6 +++-
 include/linux/compiler_attributes.h |  6 ++++
 include/linux/gfp.h                 |  4 +--
 include/linux/percpu.h              |  6 ++--
 include/linux/slab.h                | 50 ++++++++++++++++++-----------
 include/linux/vmalloc.h             | 22 ++++++-------
 6 files changed, 58 insertions(+), 36 deletions(-)

Comments

Christoph Hellwig Aug. 19, 2021, 9:09 a.m. UTC | #1

On Tue, Aug 17, 2021 at 10:08:36PM -0700, Kees Cook wrote:
> Hi,
> 
> GCC and Clang both use the "alloc_size" attribute to assist with bounds
> checking around the use of allocation functions. Add the attribute,
> adjust the Makefile to silence needless warnings, and add the hints to
> the allocators where possible. These changes have been in use for a
> while now in GrapheneOS.

Can you explain how this attribute helps?  Should we flow it through
other allocating functions?

Daniel Micay Aug. 19, 2021, 2:18 p.m. UTC | #2

It tells the compiler the function will either return NULL or an
allocation of the size specific by the parameter referenced by
alloc_size. It could also be used for functions resembling allocation
functions which aren't actually allocating. The compiler will use it
for optimization so it's extremely important that it's only used
correctly. It only really has a use on the top-level API used
externally.

The compiler uses it for __builtin_object_size which is primarily used
by FORTIFY_SOURCE and also internally by -fsanitize=object-size which
will be available for the kernel via UBSan to find bugs or as
hardening in the trapping mode. There are currently compatibility
issues (undefined out-of-bounds accesses) blocking using
-fsanitize=object-size beyond fixing those relatively benign issues to
allow using it elsewhere.

For example, it will know that kmalloc(n) returns either NULL or an
allocation of size n. A simple sample program with calloc in
userspace:

    #include <stdlib.h>
    #include <stdio.h>

    int main(void) {
        char *p = calloc(64, 1);
        if (!p) {
            return 1;
        }
        printf("%zu\n", __builtin_object_size(p, 1));
        return 0;
    }

It will also detect an out-of-bounds access via the allocation with
-fsanitize=object-size including with a runtime value as the index.

It's not as useful as it should be yet because __builtin_object_size
must return a compile-time constant. Clang has a new
__builtin_dynamic_object_size that's allowed to return a value that's
not a compile-time constant so it can work for kmalloc(n) where n is a
runtime value. It might not be quite ready for use yet but it should
be able to make it a lot more useful. GCC also seems open to adding it
too.

Christoph Lameter Aug. 25, 2021, 10:01 a.m. UTC | #3

On Thu, 19 Aug 2021, Daniel Micay wrote:

> For example, it will know that kmalloc(n) returns either NULL or an
> allocation of size n. A simple sample program with calloc in
> userspace:
>
>     #include <stdlib.h>
>     #include <stdio.h>
>
>     int main(void) {
>         char *p = calloc(64, 1);
>         if (!p) {
>             return 1;
>         }
>         printf("%zu\n", __builtin_object_size(p, 1));
>         return 0;
>     }
>
> It will also detect an out-of-bounds access via the allocation with
> -fsanitize=object-size including with a runtime value as the index.
>
> It's not as useful as it should be yet because __builtin_object_size
> must return a compile-time constant. Clang has a new
> __builtin_dynamic_object_size that's allowed to return a value that's
> not a compile-time constant so it can work for kmalloc(n) where n is a
> runtime value. It might not be quite ready for use yet but it should
> be able to make it a lot more useful. GCC also seems open to adding it
> too.

The other complication with kmalloc etc is that the slab allocators may
decided to allocate more bytes than needed because it does not support
that particular allocation size. Some functions check the allocated true
size and make use of that. See ksize().

Kees Cook Aug. 25, 2021, 4:34 p.m. UTC | #4

On Wed, Aug 25, 2021 at 12:01:42PM +0200, Christoph Lameter wrote:
> On Thu, 19 Aug 2021, Daniel Micay wrote:
> 
> > For example, it will know that kmalloc(n) returns either NULL or an
> > allocation of size n. A simple sample program with calloc in
> > userspace:
> >
> >     #include <stdlib.h>
> >     #include <stdio.h>
> >
> >     int main(void) {
> >         char *p = calloc(64, 1);
> >         if (!p) {
> >             return 1;
> >         }
> >         printf("%zu\n", __builtin_object_size(p, 1));
> >         return 0;
> >     }
> >
> > It will also detect an out-of-bounds access via the allocation with
> > -fsanitize=object-size including with a runtime value as the index.
> >
> > It's not as useful as it should be yet because __builtin_object_size
> > must return a compile-time constant. Clang has a new
> > __builtin_dynamic_object_size that's allowed to return a value that's
> > not a compile-time constant so it can work for kmalloc(n) where n is a
> > runtime value. It might not be quite ready for use yet but it should
> > be able to make it a lot more useful. GCC also seems open to adding it
> > too.
> 
> The other complication with kmalloc etc is that the slab allocators may
> decided to allocate more bytes than needed because it does not support
> that particular allocation size. Some functions check the allocated true
> size and make use of that. See ksize().

Yup, this is known. For the current iteration, this doesn't pose a
problem since the compile-time checking has very limited scope.