Message-ID: <202308161913.91369D4A@keescook>
Date:   Wed, 16 Aug 2023 20:05:34 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Alejandro Colomar <alx@...nel.org>
Cc:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: struct_size() using sizeof() vs offsetof()

On Thu, Aug 17, 2023 at 02:23:21AM +0200, Alejandro Colomar wrote:
> Hi Kees, Gustavo,

Hi!

> I've been discussing with a friend about the appropriateness of sizeof()
> vs offsetof() for calculating the size of a structure with a flexible
> array member (FAM).
> 
> After reading Jens Gustedt's blog post about it[1], we tried some tests,
> and we got some interesting results that discouraged me from using sizeof().
> See below.

When struct_size() was originally implemented this topic came up, and we
opted for potential over-estimation rather than using offsetof() which
could result in under-allocation, and using max() of two different
calculations just seemed like overkill. Additionally, almost all cases of
struct_size() was replacing a literal open-coded version of

	sizeof(*ptr) + sizeof(*ptr->array) * count

So avoiding a difference in calculation was nice too.

> But then, said friend pointed to me that the kernel uses sizeof() in
> struct_size(), and we wondered why you would have chosen it.  It's safe
> as long as you _know_ that there's no padding, or that the alignment of
> the FAM is as large as the padding (which you probably know in the kernel),
> but it seems safer to use
> 
> 	MAX(sizeof(s), offsetof(s, fam) + sizeof_member(s, fam) * count)

Ironically, this has been under careful examination recently by GCC[1]
too. Though that has mainly been looking at it from the perspective
of how __builtin_object_size() should behave in the face of the new
__counted_by attribute.

> The thing is, if there's any trailing padding in the struct, the FAM may
> overlap the padding, and the calculation with sizeof() will waste a few
> bytes, and if misused to get the location of the FAM, the problem will be
> bigger, as you'll get a wrong location.

Luckily, the _location_ of the FAM is well-defined by the compiler, so
that won't be a problem. However, yes, we can risk wasting some bytes.

> So, I just wanted to pry what and especially why the kernel chose to prefer
> a simple sizeof().

We opted for simple over complex, with the understanding that
over-allocation will be a relatively rare issue that will only waste
limited space (as opposed to potential under-allocation and risking
writing beyond the end of the region).

Here's some example I worked through:

https://godbolt.org/z/9aGjqon9v

But, yes, at the end of the day, struct_size() could be defined as
max(sizeof, offsetof-based struct-size).

Note that struct_size() has been designed to have two additional
behaviors:
 - be usable as a constant expression
 - saturate at SIZE_MAX

So as long as the max() could do the same (which it should be able to),
it'd likely be fine. I'm open to patches as long as we can validate any
binary differences found in allmodconfig builds. :)

-Kees

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626672.html

-- 
Kees Cook

Powered by blists - more mailing lists