Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <96e7cc13-1169-12ab-6dad-7624e33346ab@kernel.org>
Date:   Mon, 21 Aug 2023 15:51:22 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.1
Subject: Re: struct_size() using sizeof() vs offsetof()
Content-Language: en-US
To:     David Laight <David.Laight@ACULAB.COM>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        "linux-hardening@vger.kernel.org" <linux-hardening@vger.kernel.org>
References: <74e8cf91-d095-33e3-c548-34d80b691089@kernel.org>
 <202308161913.91369D4A@keescook>
 <09b4a2ce-da74-3a19-6961-67883f634d98@kernel.org>
 <f12c6f14-66c4-6afb-e768-fa7abcfd1bbc@embeddedor.com>
 <df8fdd4a-490f-6b2a-03b6-0333e3302dce@kernel.org>
 <d3c4c953c9a742ae98ae9b9036561b38@AcuMS.aculab.com>
From:   Alejandro Colomar <alx@kernel.org>
Organization: Linux
In-Reply-To: <d3c4c953c9a742ae98ae9b9036561b38@AcuMS.aculab.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 2023-08-21 10:38, David Laight wrote:
> From: Alejandro Colomar <alx@kernel.org>
>> Sent: Thursday, August 17, 2023 7:38 PM
>>
>> Hi Gustavo,
>>
>> On 2023-08-17 18:05, Gustavo A. R. Silva wrote:
>>>
>>>> -               tp_c =3D kzalloc(sizeof(*tp_c), GFP_KERNEL);
>>>> +               tp_c =3D kzalloc(struct_size(tp_c, hlist->ht, 1), GF=
P_KERNEL);
>>>
>>> I just sent a fix[1].
>>>
>>> Thanks for reporting this! :)
>=20
> Perhaps struct_size() should include an assertion that:
> 	(offsetof(type, field[8]) > sizeof (type))
> That will ensure that field is an array member

There's already an assertion that the field in struct_size() is an
array:

$ grepc struct_size include/
include/linux/overflow.h:291:
#define struct_size(p, member, count)					\
	__builtin_choose_expr(__is_constexpr(count),			\
		sizeof(*(p)) + flex_array_size(p, member, count),	\
		size_add(sizeof(*(p)), flex_array_size(p, member, count)))
$ grepc flex_array_size include/
include/linux/overflow.h:275:
#define flex_array_size(p, member, count)				\
	__builtin_choose_expr(__is_constexpr(count),			\
		(count) * sizeof(*(p)->member) + __must_be_array((p)->member),	\
		size_mul(count, sizeof(*(p)->member) + __must_be_array((p)->member)))


Notice the must_be_array() there.


> and reasonably
> near the end of the structure.

I did add a must_be_zero_sizeof() assertion to my implementation of
sizeof_fam0(), so I think that's a reasonable assertion that it's
really a FAM.  You can still add a zero-length array in the middle
of a struct and pass those two assertions, but it's unlikely.

>=20
> A more complex calculation (using _Alignof(type) and the offset/size
> of field) could be used.
> But I don't think you can actually detect it is field[] (or even the
> last member).

I'm thinking now that you can plug an assertion that offsetof_fam()
is >=3D sizeof(struct) - alignof(struct).  That should make it even
harder to pass all the assertions without really having the field at
the end.  A [0] at the end of a structure would still pass all those
assertions, but linters probably catch those.  So I think it's a
pretty robust assertion.

Cheers,
Alex

>=20
> 	David
>=20
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, M=
K1 1PT, UK
> Registration No: 1397386 (Wales)

--=20
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5