linux-kernel - Re: struct_size() using sizeof() vs offsetof()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <96e7cc13-1169-12ab-6dad-7624e33346ab@kernel.org>
Date:   Mon, 21 Aug 2023 15:51:22 +0200
From:   Alejandro Colomar <alx@...nel.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        "linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: struct_size() using sizeof() vs offsetof()

On 2023-08-21 10:38, David Laight wrote:
> From: Alejandro Colomar <alx@...nel.org>
>> Sent: Thursday, August 17, 2023 7:38 PM
>>
>> Hi Gustavo,
>>
>> On 2023-08-17 18:05, Gustavo A. R. Silva wrote:
>>>
>>>> -               tp_c = kzalloc(sizeof(*tp_c), GFP_KERNEL);
>>>> +               tp_c = kzalloc(struct_size(tp_c, hlist->ht, 1), GFP_KERNEL);
>>>
>>> I just sent a fix[1].
>>>
>>> Thanks for reporting this! :)
> 
> 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.

> 
> 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 >= 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

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

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