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] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 21 Aug 2023 15:45:37 +0200
From:   Alejandro Colomar <alx@...nel.org>
To:     David Laight <David.Laight@...LAB.COM>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>
Subject: Re: struct_size() using sizeof() vs offsetof()

Hi David,

On 2023-08-21 10:16, David Laight wrote:
> From: Alejandro Colomar
>> Sent: Thursday, August 17, 2023 1:23 AM
>>
>> 	strcpy(s->fam, "Hello, sizeof!");
>> 	p = (char *) s + sizeof(struct s);
>> 	puts(p);
> 
> Why on earth would you expect the above to do anything sensible?

This trivial example may seem unreasonable, but I've seen code that
does something like that (but more complex).  Not in the kernel, but
in an nginx subproject:

<https://github.com/nginx/unit/blob/47ff51009fa05d83bb67cd5db16829ab4c0081d7/src/wasm/nxt_wasm.c#L108>
<https://github.com/nginx/unit/blob/47ff51009fa05d83bb67cd5db16829ab4c0081d7/src/wasm/nxt_wasm.c#L160>

It uses pointer arithmetic with sizeof to get the offset of the FAM,
instead of calling it by its name.

> 
> It is a shame you can just use offsetof(type, member[count + 1]).
> That is fine for constants, but the C language requires offsetof()
> to be a compile-time constant - I can't help feeling the standards
> body didn't consider non-constant array offsets.

This helped catch a bug last week, so I think it's good that the
standard disallows it.

You can always write a macro offsetof_fam(type, fam, n) which does
that.  In fact, I've written it, and will be part of the patch that
I'll propose.  It is much safer than if offsetof() would just
accept that, as I can embed some static assertions within that
macro.

Here's a look at the file I've been testing before submitting a patch.
A lot of what you'll see here is similar to what I pretend to send in
a patch.


$ cat alx_cdefs.h 
/* Copyright (C) 2023 Alejandro Colomar <alx@...nel.org> */
/* SPDX-License-Identifier: GPL-3.0-or-later */

#ifndef ALX_CDEFS_H_INCLUDED_
#define ALX_CDEFS_H_INCLUDED_


#include <stddef.h>
#include <sys/param.h>


#define sizeof_array(a)      (sizeof(a) + must_be_array(a))
#define nitems(a)            (sizeof_array(a) / sizeof((a)[0]))
#define memberof(T, member)  ((T){}.member)

#define sizeof_incomplete(x)                                                  \
(                                                                             \
	sizeof(                                                               \
		struct {                                                      \
			max_align_t  a;                                       \
			typeof(x)    inc;                                     \
		}                                                             \
	)                                                                     \
	- sizeof(max_align_t)                                                 \
)

#define sizeof_fam0(T, fam)  (sizeof(memberof(T, fam[0])) + must_be_fam(T, fam))
#define sizeof_fam(T, fam, n)     (sizeof_fam0(T, fam) * (n))
#define offsetof_fam(T, fam, n)   (offsetof(T, fam) + sizeof_fam(T, fam, n))
#define sizeof_struct(T, fam, n)  MAX(sizeof(T), offsetof_fam(T, fam, n))


#define is_zero_sizeof(z)     (sizeof_incomplete(z) == 0)
#define is_same_type(a, b)    __builtin_types_compatible_p(a, b)
#define is_same_typeof(a, b)  is_same_type(typeof(a), typeof(b))
#define is_array(a)           (!is_same_typeof(a, &(a)[0]))


#define must_be(e)                                                            \
(                                                                             \
	0 * (int) sizeof(                                                     \
		struct {                                                      \
			_Static_assert(e, "");                                \
			int ISO_C_forbids_a_struct_with_no_members_;          \
		}                                                             \
	)                                                                     \
)


#define must_be_array(a)        must_be(is_array(a))
#define must_be_zero_sizeof(z)  must_be(is_zero_sizeof(z))

#define must_be_fam(T, fam)                                               \
    (must_be_array(memberof(T, fam)) + must_be_zero_sizeof(memberof(T, fam)))


#endif /* ALX_CDEFS_H_INCLUDED_ */


> (The compiler for a well known OS won't compile that (or anything
> that looks like it) even for a constant array subscript!)
> 
> The actual problem with using offsetof() is that you might end
> up with something smaller than the structure size.
> (When the variable sized array is smaller than the padding.)

That's why MAX().

> 
> While max() will generate a constant for constant input, it
> will be a real compare for non-constant input.

If the input was non-constant, then it would already have been
non-constant with the current code.  I don't think MAX() will
make it worse.

> 
> 	David

Cheers,
Alex


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