All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
To: Siddhesh Poyarekar <siddhesh@gotplt.org>,
	Kees Cook <kees@kernel.org>,
	"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Cc: Jakub Jelinek <jakub@redhat.com>,
	Richard Biener <richard.guenther@gmail.com>,
	gcc-patches@gcc.gnu.org, Martin Uecker <uecker@tugraz.at>,
	josmyers@redhat.com, Bill Wendling <morbo@google.com>,
	linux-hardening@vger.kernel.org,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>
Subject: Re: [PATCH v2] tree-object-size: Fix type-1 size for FAM subobjects under -fstrict-flex-arrays=3 [PR126975]
Date: Mon, 21 Sep 2026 20:48:56 +0900	[thread overview]
Message-ID: <2df2f47d-5bef-4272-936b-13cfb350aac7@embeddedor.com> (raw)
In-Reply-To: <7f43c0b9-2ea7-4038-82c5-ca01aa2c1309@gotplt.org>



On 9/19/26 22:27, Siddhesh Poyarekar wrote:
> On 2026-09-19 00:22, Kees Cook wrote:
>> On Thu, Sep 03, 2026 at 08:02:20PM -0600, Gustavo A. R. Silva wrote:
>>> For a pointer to a subobject whose record/union type ends in a flexible-array
>>> member (directly, or through a trailing nested struct), addr_object_size()
>>> walked up to the enclosing object (v = TREE_OPERAND (v, 0)) instead of
>>> measuring the referenced subobject.  __builtin_object_size() and
>>> __builtin_dynamic_object_size() type 1 therefore returned the whole-object
>>> size, collapsing type 1 onto type 0 and losing the distinction between
>>> &p->inner and p.  FORTIFY_SOURCE relies on the type-1 distinction, so this
>>> weakens its bounds checks.
>>
>> Looking at the PR:
>>
>> struct flex {
>>    size_t count;
>>    char fam[] __attribute__((counted_by(count)));
>> };                    /* sizeof(struct flex) == 8  */
>>
>> struct outer {
>>    int hdr;
>>    struct flex inner;
>> };                    /* sizeof(struct outer) == 16 */
>>
>> int main (void)
>> {
>>    struct outer *p = __builtin_malloc (sizeof (*p) + 48); /* 64-byte object */
>>
>>    /* This is a bug. */
>>    expect(__builtin_object_size (&p->inner, 1), sizeof(p->inner));
>>    expect(__builtin_dynamic_object_size (&p->inner, 1), sizeof(p->inner));
>> ...
>> }
>>
>> Before:
>>
>> WAT: __builtin_object_size (&p->inner, 1) == 56 (expected 8)
>> WAT: __builtin_dynamic_object_size (&p->inner, 1) == 56 (expected 8)
>>
>> After:
>>
>> ok:  __builtin_object_size (&p->inner, 1) == 8
>> ok:  __builtin_dynamic_object_size (&p->inner, 1) == 8
>>
>>
>> This makes sense to me, the dynamic size of "fam" is ambiguous between
>> being 0 or 48 (from the "alloc_size" attribute), so type 1 chooses the
>> smaller.
> 
> I think you're confusing type 1 with type 2; type 1 is the *maximum* subobject size, not the minimum.  The whole object and subobject size distinction is only 
> material if there are members after inner, which is technically "supported" by the complier as an extension, but is a terrible idea for members with FAM.
> 
> Here are the different possibilities for &p->inner:
> 
> 1. If the allocation via __builtin_malloc is visible: same result for types 0, 1, 2 and 3, i.e. allocated size - offsetof (struct outer, inner)

When the allocation is visible and the FAM is annotated with counted_by, we use
the information provided by counted_by. See commit 6f17933548fc ("Use the
.ACCESS_WITH_SIZE in builtin object size.")

BTW, I recently filed this related issue:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127234

> 
> 2. If the __builtin_malloc is not visible and there's no counted_by annotation:
>    - type 0 and type 1: cannot determine size, since FAM could be any arbitrary size. So, -1
>    - type 2 and type 3: sizeof (&p->inner), since that's the minimum estimate.
> 
> 3. if the __builtin_malloc is not visible and FAM has __counted_by__ annotation: same result for all types: sizeof (&p->inner) + p->count
> 
>> I would, however, expect the use of counted_by to change it. For
>> example, if it were this:
>>
>> struct flex {
>>    size_t count;
>>    char fam[] __attribute__((counted_by(count)));
>> };
>> ...
>>    struct outer *p = __builtin_malloc (sizeof (*p) + 48); /* 64-byte object */
>>    p->inner.count = 48;
>>
>> I would expect the results to be:
>>
>> ok:  __builtin_object_size (&p->inner, 1) == 8        /* compile-time size */
>> ok:  __builtin_dynamic_object_size (&p->inner, 1) == 56 /* run-time size */

I agree with counted_by changing things here. This is actually something
that Jakub mentioned in a comment to v1 of this patch:

"What we certainly can change is the behavior when
-fstrict-flex-arrays unless it already behaves the expected way,
and perhaps also when the flex array has counted_by attribute."

See: https://lore.kernel.org/linux-hardening/aogVH8UtICMP1Ihv@tucnak/#t

BTW, I have a patch ready for PR127234 (the missed counted_by bug) where
I implemented some helper functions to get the size of the object based
on the information provided by counted_by. I could use those in this PR. :)

Thanks
-Gustavo

>>
>> As the known size of p->inner is everything contained by "inner" and
>> that now includes the 48 bytes of "fam".
> __builtin_object_size is not the compile time size, it is a runtime constant size estimate, with the `type` argument deciding if the size estimate is on the 
> maximum or minimum end, and if it considers the whole object or only the immediate containing subobject that the input pointer points to.  In the presence of 
> FAMs I think sizeof (i.e. the compile time size) should be seen as a minimum size estimate for the subobject, if you want to make an equivalence with 
> __builtin_object_size.
> 
> Similarly, __builtin_dynamic_object_size is exactly the same as __builtin_object_size, except that it can return non-constant expressions too, not just constants.
> 
> Thanks,
> Sid


  reply	other threads:[~2026-09-21 11:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-04  2:02 [PATCH v2] tree-object-size: Fix type-1 size for FAM subobjects under -fstrict-flex-arrays=3 [PR126975] Gustavo A. R. Silva
2026-09-17  6:58 ` Gustavo A. R. Silva
2026-09-18 16:19 ` Siddhesh Poyarekar
2026-09-19  4:22 ` Kees Cook
2026-09-19 13:27   ` Siddhesh Poyarekar
2026-09-21 11:48     ` Gustavo A. R. Silva [this message]
2026-09-21 12:46       ` Siddhesh Poyarekar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2df2f47d-5bef-4272-936b-13cfb350aac7@embeddedor.com \
    --to=gustavo@embeddedor.com \
    --cc=garsilva@embeddedor.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=gustavoars@kernel.org \
    --cc=jakub@redhat.com \
    --cc=josmyers@redhat.com \
    --cc=kees@kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=morbo@google.com \
    --cc=richard.guenther@gmail.com \
    --cc=siddhesh@gotplt.org \
    --cc=uecker@tugraz.at \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.