Messages in this thread | | | Date | Wed, 16 Aug 2023 20:05:34 -0700 | From | Kees Cook <> | 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
| |