Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Thu, Oct 05, 2023 at 04:08:52PM -0400, Siddhesh Poyarekar wrote: > 2. How would you handle signedness of the size field? The size gets > converted to sizetype everywhere it is used and overflows/underflows may > produce interesting results. Do you want to limit the types to unsigned or > do you want to add a disclaimer in the docs? The former seems like the > *right* thing to do given that it is a new feature; best to enforce the > cleaner habit at the outset. The Linux kernel has a lot of "int" counters, so the goal is to catch negative offsets just like too-large offsets at runtime with the sanitizer and report 0 for __bdos. Refactoring all these to be unsigned is going to take time since at least some of them use the negative values as special values unrelated to array indexing. :( So, perhaps if unsigned counters are worth enforcing, can this be a separate warning the kernel can turn off initially? -Kees -- Kees Cook
Re: Improve -Wflex-array-member-not-at-end changes.html wording |Plus: and warning bug? (was: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.)
On Thu, Oct 19, 2023 at 08:49:00PM +, Qing Zhao wrote: > > On Sep 25, 2023, at 2:24 PM, Tobias Burnus wrote: > > Secondly, if this is deprecated, shouldn't then the warning enabled by, > > e.g., -Wall or made > > otherwise more prominent? (-std=?) - Currently, one either has to find the > > new flag or use > > -pedantic. > > Yes, agreed, However, I think that it might be better to delay this to next > GCC release by giving users plenty time to fix all the > -Wflex-array-member-not-at-end warnings. As I know, linux kernel exposed a > lot of warnings when adding -Wflex-array-member-not-at-end, and kernel people > are trying to fix all these warnings in the source base. Yeah, for the code bases that use flexible arrays (C99 or GNU Extension), there are cases where they're used as intentional implicit unions, and the refactoring to make them unambiguous is non-trivial. I think it may need to be some time before -Wflex-array-member-not-at-end ends up in -Wall. I gave some examples of this code pattern (and potential solutions) here: https://lore.kernel.org/lkml/202310161249.43FB42A6@keescook -- Kees Cook
Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Oct 18, 2023 at 09:11:43PM +, Qing Zhao wrote: > As I replied to Martin in another email, I plan to do the following to > resolve this issue: > > 1. No specification for signed or unsigned for counted_by field. > 2. Add a sanitizer option -fsanitize=counted-by-bound to catch the cases when > the size of the counted-by is not positive. I don't understand why this needs to be a runtime sanitizer. The signedness is known at compile time, so I would expect a -W option. Or do you mean you'd split up -fsanitize=bounds between unsigned and signed indexes? I'd find that kind of awkward for the kernel... but I feel like I've misunderstood something. :) -Kees -- Kees Cook
Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Oct 20, 2023 at 11:50:11AM +0200, Martin Uecker wrote:
> Am Donnerstag, dem 19.10.2023 um 16:33 -0700 schrieb Kees Cook:
> > On Wed, Oct 18, 2023 at 09:11:43PM +, Qing Zhao wrote:
> > > As I replied to Martin in another email, I plan to do the following to
> > > resolve this issue:
> > >
> > > 1. No specification for signed or unsigned for counted_by field.
> > > 2. Add a sanitizer option -fsanitize=counted-by-bound to catch the cases
> > > when the size of the counted-by is not positive.
> >
> > I don't understand why this needs to be a runtime sanitizer. The
> > signedness is known at compile time, so I would expect a -W option.
>
> The signedness of the type but not of the value.
>
> But I would not want to have a warning for signed
> counter types by default because I would prefer
> to use signed types (for various reasons including
> better overflow detection).
>
> > Or
> > do you mean you'd split up -fsanitize=bounds between unsigned and signed
> > indexes? I'd find that kind of awkward for the kernel... but I feel like
> > I've misunderstood something. :)
> >
> > -Kees
>
> The idea would be to detect at run-time the case
> if x->buf is used at a time where x->counter
> is negative and also when x->counter * sizeof(x->buf[0])
> overflows or is too big.
>
> This would be similar to
>
> int a[n];
>
> where it is detected at run-time if n is not-positive.
Right. I guess what I mean to say is that I would expect this case to
already be caught by -fsanitize=bounds -- I don't see a reason to add an
additional sanitizer option.
struct foo {
int count;
int array[] __counted_by(count);
};
foo->count = 5;
foo->array[0] = 1; // ok
foo->array[10] = 1; // -fsanitize=bounds will catch this
foo->array[-10] = 1;// -fsanitize=bounds will catch this too
--
Kees Cook
Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Oct 20, 2023 at 09:54:05PM +0200, Martin Uecker wrote:
> Am Freitag, dem 20.10.2023 um 18:48 + schrieb Qing Zhao:
> >
> > > On Oct 20, 2023, at 2:34 PM, Kees Cook wrote:
> > >
> > > On Fri, Oct 20, 2023 at 11:50:11AM +0200, Martin Uecker wrote:
> > > > Am Donnerstag, dem 19.10.2023 um 16:33 -0700 schrieb Kees Cook:
> > > > > On Wed, Oct 18, 2023 at 09:11:43PM +, Qing Zhao wrote:
> > > > > > As I replied to Martin in another email, I plan to do the following
> > > > > > to resolve this issue:
> > > > > >
> > > > > > 1. No specification for signed or unsigned for counted_by field.
> > > > > > 2. Add a sanitizer option -fsanitize=counted-by-bound to catch the
> > > > > > cases when the size of the counted-by is not positive.
> > > > >
> > > > > I don't understand why this needs to be a runtime sanitizer. The
> > > > > signedness is known at compile time, so I would expect a -W option.
> > > >
> > > > The signedness of the type but not of the value.
> > > >
> > > > But I would not want to have a warning for signed
> > > > counter types by default because I would prefer
> > > > to use signed types (for various reasons including
> > > > better overflow detection).
> > > >
> > > > > Or
> > > > > do you mean you'd split up -fsanitize=bounds between unsigned and
> > > > > signed
> > > > > indexes? I'd find that kind of awkward for the kernel... but I feel
> > > > > like
> > > > > I've misunderstood something. :)
> > > > >
> > > > > -Kees
> > > >
> > > > The idea would be to detect at run-time the case
> > > > if x->buf is used at a time where x->counter
> > > > is negative and also when x->counter * sizeof(x->buf[0])
> > > > overflows or is too big.
> > > >
> > > > This would be similar to
> > > >
> > > > int a[n];
> > > >
> > > > where it is detected at run-time if n is not-positive.
> > >
> > > Right. I guess what I mean to say is that I would expect this case to
> > > already be caught by -fsanitize=bounds -- I don't see a reason to add an
> > > additional sanitizer option.
> > >
> > > struct foo {
> > > int count;
> > > int array[] __counted_by(count);
> > > };
> > >
> > > foo->count = 5;
> > > foo->array[0] = 1; // ok
> > > foo->array[10] = 1; // -fsanitize=bounds will catch this
> > > foo->array[-10] = 1;// -fsanitize=bounds will catch this too
> > >
> > >
> >
> > just checked this testing case with my GCC, and YES, -fsanitize=bounds
> > indeed caught this error:
> >
> > ttt_1.c:31:12: runtime error: index 10 out of bounds for type 'char [*]'
> > ttt_1.c:32:12: runtime error: index -10 out of bounds for type 'char [*]’
> >
>
> Yes, but I thought we were discussing the case where count is
> set to a negative value:
>
> foo->count = -1;
> int x = foo->array[3]; // UBSan should diagnose this
Oh right, I keep thinking about it backwards.
Yeah, we can't trap the "count" assignment, because it may be getting used
for other purposes. But yeah, access to "array" should trap if "count"
is negative.
> And also the case when foo->array becomes too big.
How do you mean?
--
Kees Cook
Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Mon, Oct 23, 2023 at 09:57:45PM +0200, Martin Uecker wrote:
> Am Montag, dem 23.10.2023 um 12:52 -0700 schrieb Kees Cook:
> > On Fri, Oct 20, 2023 at 09:54:05PM +0200, Martin Uecker wrote:
> > > Am Freitag, dem 20.10.2023 um 18:48 + schrieb Qing Zhao:
> > > >
> > > > > On Oct 20, 2023, at 2:34 PM, Kees Cook wrote:
> > > > >
> > > > > On Fri, Oct 20, 2023 at 11:50:11AM +0200, Martin Uecker wrote:
> > > > > > Am Donnerstag, dem 19.10.2023 um 16:33 -0700 schrieb Kees Cook:
> > > > > > > On Wed, Oct 18, 2023 at 09:11:43PM +, Qing Zhao wrote:
> > > > > > > > As I replied to Martin in another email, I plan to do the
> > > > > > > > following to resolve this issue:
> > > > > > > >
> > > > > > > > 1. No specification for signed or unsigned for counted_by field.
> > > > > > > > 2. Add a sanitizer option -fsanitize=counted-by-bound to catch
> > > > > > > > the cases when the size of the counted-by is not positive.
> > > > > > >
> > > > > > > I don't understand why this needs to be a runtime sanitizer. The
> > > > > > > signedness is known at compile time, so I would expect a -W
> > > > > > > option.
> > > > > >
> > > > > > The signedness of the type but not of the value.
> > > > > >
> > > > > > But I would not want to have a warning for signed
> > > > > > counter types by default because I would prefer
> > > > > > to use signed types (for various reasons including
> > > > > > better overflow detection).
> > > > > >
> > > > > > > Or
> > > > > > > do you mean you'd split up -fsanitize=bounds between unsigned and
> > > > > > > signed
> > > > > > > indexes? I'd find that kind of awkward for the kernel... but I
> > > > > > > feel like
> > > > > > > I've misunderstood something. :)
> > > > > > >
> > > > > > > -Kees
> > > > > >
> > > > > > The idea would be to detect at run-time the case
> > > > > > if x->buf is used at a time where x->counter
> > > > > > is negative and also when x->counter * sizeof(x->buf[0])
> > > > > > overflows or is too big.
> > > > > >
> > > > > > This would be similar to
> > > > > >
> > > > > > int a[n];
> > > > > >
> > > > > > where it is detected at run-time if n is not-positive.
> > > > >
> > > > > Right. I guess what I mean to say is that I would expect this case to
> > > > > already be caught by -fsanitize=bounds -- I don't see a reason to add
> > > > > an
> > > > > additional sanitizer option.
> > > > >
> > > > > struct foo {
> > > > > int count;
> > > > > int array[] __counted_by(count);
> > > > > };
> > > > >
> > > > > foo->count = 5;
> > > > > foo->array[0] = 1; // ok
> > > > > foo->array[10] = 1; // -fsanitize=bounds will catch this
> > > > > foo->array[-10] = 1;// -fsanitize=bounds will catch this too
> > > > >
> > > > >
> > > >
> > > > just checked this testing case with my GCC, and YES, -fsanitize=bounds
> > > > indeed caught this error:
> > > >
> > > > ttt_1.c:31:12: runtime error: index 10 out of bounds for type 'char [*]'
> > > > ttt_1.c:32:12: runtime error: index -10 out of bounds for type 'char
> > > > [*]’
> > > >
> > >
> > > Yes, but I thought we were discussing the case where count is
> > > set to a negative value:
> > >
> > > foo->count = -1;
> > > int x = foo->array[3]; // UBSan should diagnose this
> >
> > Oh right, I keep thinking about it backwards.
> >
> > Yeah, we can't trap the "count" assignment, because it may be getting used
> > for other purposes. But yeah, access to "array" should trap if "count"
> > is negative.
> >
> > > And also the case when foo->array becomes too big.
> >
> > How do you mean?
>
> count * sizeof(member) could overflow or otherwise be
> bigger than allowed.
Ah! Yes.
foo->count = SIZE_MAX;
foo->array[0]; // UBSan diagnose:
// SIZE_MAX * sizeof(int) is larger than can be represented
>
> Martin
>
>
--
Kees Cook
Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Tue, Oct 24, 2023 at 07:51:55PM -0400, Siddhesh Poyarekar wrote: > Yes, that's the tradeoff. However, maybe this is the point where Kees jumps > in and say the kernel doesn't really care as much or something like that :) "I only care about -O2" :) -- Kees Cook
Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Oct 25, 2023 at 01:27:29PM +, Qing Zhao wrote: > A. Add an additional argument, the size parameter, to __bdos, > A.1, during FE; > A.2, during gimplification phase; I just wanted to clarify that this is all just an "internal" detail, yes? i.e. the __bdos() used by in C code is unchanged? For example, the Linux kernel can still use __bdos() without knowing the count member ahead of time (otherwise it kind of defeats the purpose). -- Kees Cook
Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Oct 25, 2023 at 10:27:41PM +, Qing Zhao wrote: > > > > On Oct 25, 2023, at 6:06 PM, Kees Cook wrote: > > > > On Wed, Oct 25, 2023 at 01:27:29PM +, Qing Zhao wrote: > >> A. Add an additional argument, the size parameter, to __bdos, > >> A.1, during FE; > >> A.2, during gimplification phase; > > > > I just wanted to clarify that this is all just an "internal" detail, > > yes? > > YES! Okay, I thought so, but I just wanted to double-check. :) > > For example, the Linux kernel can still use __bdos() without knowing > > the count member ahead of time (otherwise it kind of defeats the purpose). > Don’t quite understand this, could you clarify? I was just trying to explain why a chance would be a problem. But it doesn't matter, so nevermind. :) > (Anyway, the bottom line is no change to the user interface, we just discuss > the internal implementation inside GCC) -:) Great! I'll go back to lurking. :) Thanks! -- Kees Cook
Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Thu, Oct 26, 2023 at 10:15:10AM +0200, Martin Uecker wrote:
> but not this:
>
> char *p = &x->buf;
> x->count = 1;
> p[10] = 1; // !
This seems fine to me -- it's how I'd expect it to work: "10" is beyond
"1".
> (because the pointer is passed around the
> store to the counter)
>
> and also here the second store is then irrelevant
> for the access:
>
> x->count = 10;
> char* p = &x->buf;
> ...
> x->count = 1; // somewhere else
>
> p[9] = 1; // ok, because count matter when buf was accesssed.
This is less great, but I can understand why it happens. "p" loses the
association with "x". It'd be nice if "p" had to way to retain that it
was just an alias for x->buf, so future p access would check count.
But this appears to be an existing limitation in other areas where an
assignment will cause the loss of object association. (I've run into
this before.) It's just more surprising in the above example because in
the past the loss of association would cause __bdos() to revert back to
"SIZE_MAX" results ("I don't know the size") rather than an "outdated"
size, which may get us into unexpected places...
> IMHO this makes sense also from the user side and
> are the desirable semantics we discussed before.
>
> But can you take a look at this?
>
>
> This should simulate it fairly well:
> https://godbolt.org/z/xq89aM7Gr
>
> (the call to the noinline function would go away,
> but not necessarily its impact on optimization)
Yeah, this example should be a very rare situation: a leaf function is
changing the characteristics of the struct but returning a buffer within
it to the caller. The more likely glitch would be from:
int main()
{
struct foo *f = foo_alloc(7);
char *p = FAM_ACCESS(f, size, buf);
printf("%ld\n", __builtin_dynamic_object_size(p, 0));
test1(f); // or just "f->count = 10;" no function call needed
printf("%ld\n", __builtin_dynamic_object_size(p, 0));
return 0;
}
which reports:
7
7
instead of:
7
10
This kind of "get an alias" situation is pretty common in the kernel
as a way to have a convenient "handle" to the array. In the case of a
"fill the array without knowing the actual final size" code pattern,
things would immediately break:
struct foo *f;
char *p;
int i;
f = alloc(maximum_possible);
f->count = 0;
p = f->buf;
for (i; data_is_available() && i < maximum_possible; i++) {
f->count ++;
p[i] = next_data_item();
}
Now perhaps the problem here is that "count" cannot be used for a count
of "logically valid members in the array" but must always be a count of
"allocated member space in the array", which I guess is tolerable, but
isn't ideal -- I'd like to catch logic bugs in addition to allocation
bugs, but the latter is certainly much more important to catch.
--
Kees Cook
Re: HELP: Will the reordering happen? Re: [V3][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Oct 27, 2023 at 03:10:22PM +, Qing Zhao wrote:
> Since the dynamic array support is quite important to the kernel (is this
> true, Kees? ),
> We might need to include such support into our design in the beginning.
tl;dr: We don't need "dynamic array support" in the 1st version of __counted_by
I'm not sure it's as strong as "quite important", but it is a code
pattern that exists. The vast majority of FAM usage is run-time fixed,
in the sense that the allocation matches the usage. Only sometimes do we
over-allocate and then slowly fill it up like I've shown.
So really my thoughts on this are to bring light to the usage pattern
in the hopes that we don't make it an impossible thing to do. And if
it's a limitation of the initial version of __counted_by, the kernel can
still use it: it will just need to use __counted_by strictly for
allocation sizes, not "usage" size:
struct foo {
int allocated;
int used;
int array[] __counted_by(allocated); // would nice to use "used"
};
struct foo *p;
p = alloc(sizeof(*p) + sizeof(*p->array) * max_items);
p->allocated = max_items;
p->used = 0;
while (data_available())
p->array[++p->used] = next_datum();
With this, we'll still catch p->array accesses beyond "allocated",
but other code in the kernel won't catch "invalid data" accesses for
p->array beyond "used". (i.e. we still have memory corruption protection,
just not logic error protection.)
We can deal with aliasing in the future if we want to expand to catching
logic errors.
I should not that we don't get logic error protection from things like
ARM's Memory Tagging Extension either -- it only tracks allocation size
(and is very expensive to change as the "used" part of an allocation
grows), so this isn't an unreasonable condition for __counted_by to
require as well.
--
Kees Cook
Re: [PATCH v7 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Mar 20, 2024 at 01:15:13PM +, Qing Zhao wrote: > This is the 7th version of the patch. This happily builds the Linux kernel with all its hundreds of counted_by annotations. My behavioral regression tests all pass too: # PASSED: 19 / 19 tests passed. # Totals: pass:17 fail:0 xfail:2 xpass:0 skip:0 error:0 Thanks! -Kees -- Kees Cook
Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Mar 29, 2024 at 12:09:15PM -0600, Tom Tromey wrote: > >>>>> Qing Zhao writes: > > > This is the 8th version of the patch. > > > compare with the 7th version, the difference are: > > [...] > > Hi. I was curious to know if the information supplied by this attribute > shows up in the DWARF. It would be good if it did, because that would > let gdb correctly print these arrays without user intervention. Does DWARF have such an annotation? Regardless, I think this could be a future patch to not hold up landing the initial feature. -- Kees Cook
Re: [PATCH v8 0/5] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Mar 29, 2024 at 04:06:58PM +, Qing Zhao wrote: > This is the 8th version of the patch. Thanks for the updated version! I've done a full Linux kernel build and run through my behavioral regression test suite. Everything is working as expected. -Kees -- Kees Cook
Re: Question on -fwrapv and -fwrapv-pointer
On Thu, Feb 15, 2024 at 12:32:17AM -0800, Fangrui Song wrote: > On Fri, Sep 15, 2023 at 11:43 AM Kees Cook via Gcc-patches > wrote: > > > > On Fri, Sep 15, 2023 at 05:47:08PM +, Qing Zhao wrote: > > > > > > > > > > On Sep 15, 2023, at 1:26 PM, Richard Biener > > > > wrote: > > > > > > > > > > > > > > > >> Am 15.09.2023 um 17:37 schrieb Qing Zhao : > > > >> > > > >> > > > >> > > > >>>> On Sep 15, 2023, at 11:29 AM, Richard Biener > > > >>>> wrote: > > > >>>> > > > >>>> > > > >>>> > > > >>>>> Am 15.09.2023 um 17:25 schrieb Qing Zhao : > > > >>>> > > > >>>> > > > >>>> > > > >>>>> On Sep 15, 2023, at 8:41 AM, Arsen Arsenović > > > >>>>> wrote: > > > >>>>> > > > >>>>> > > > >>>>> Qing Zhao writes: > > > >>>>> > > > >>>>>> Even though unsigned integer overflow is well defined, it might be > > > >>>>>> unintentional, shall we warn user about this? > > > >>>>> > > > >>>>> This would be better addressed by providing operators or functions > > > >>>>> that > > > >>>>> do overflow checking in the language, so that they can be explicitly > > > >>>>> used where overflow is unexpected. > > > >>>> > > > >>>> Yes, that will be very helpful to prevent unexpected overflow in the > > > >>>> program in general. > > > >>>> However, this will mainly benefit new codes. > > > >>>> > > > >>>> For the existing C codes, especially large applications, we still > > > >>>> need to identify all the places > > > >>>> Where the overflow is unexpected, and fix them. > > > >>>> > > > >>>> One good example is linux kernel. > > > >>>> > > > >>>>> One could easily imagine a scenario > > > >>>>> where overflow is not expected in some region of code but is in the > > > >>>>> larger application. > > > >>>> > > > >>>> Yes, that’s exactly the same situation Linux kernel faces now, the > > > >>>> unexpected Overflow and > > > >>>> expected wrap-around are mixed together inside one module. > > > >>>> It’s hard to detect the unexpected overflow under such situation > > > >>>> based on the current GCC. > > > >>> > > > >>> But that’s hardly GCCs fault nor can GCC fix that in any way. Only > > > >>> the programmer can distinguish both cases. > > > >> > > > >> Right, compiler cannot fix this. > > > >> But can provide some tools to help the user to detect this more > > > >> conveniently. > > > >> > > > >> Right now, GCC provides two set of options for different types: > > > >> > > > >> A. Turn the overflow to expected wrap-around (remove UB); > > > >> B. Detect overflow; > > > >> > > > >> AB > > > >> remove UB-fsanitize=… > > > >> signed -fwrapvsigned-integer-overflow > > > >> pointer -fwrapv-pointerpointer-overflow (broken in Clang) > > > >> > > > >> However, Options in A and B excluded with each other. They cannot mix > > > >> together for a single file. > > > >> > > > >> What’s requested from Kernel is: > > > >> > > > >> compiler needs to provide a functionality that can mix these two > > > >> together for a file. > > > >> > > > >> i.e, apply A (convert UB to defined behavior WRAP-AROUND) only to part > > > >> of the program. And then add -fsnaitize=*overflow to detect all other > > > >> Unexpected overflows in the program. > > Yes, I believe combining A and B should be allowed. > > > > >> This is currently missing from GCC, I guess? > > > > > > > > How can GCC know which part of the program wants
Re: [PATCH v6 0/5]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Feb 16, 2024 at 07:47:18PM +, Qing Zhao wrote: > This is the 6th version of the patch. Thanks! I've tested this and it meets all the current behavioral expectations I've got: https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c Additionally, this builds the Linux kernel where we have almost 300 instances of "counted_by" attributes. Yay! -Kees -- Kees Cook
Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Jan 24, 2024 at 12:29:51AM +, Qing Zhao wrote:
> This is the 4th version of the patch.
Thanks very much for this!
I tripped over an unexpected behavioral change that the Linux kernel
depends on:
__builtin_types_compatible_p() no longer treats an array marked with
counted_by as different from that array's decayed pointer. Specifically,
the kernel uses these macros:
/*
* Force a compilation error if condition is true, but also produce a
* result (of value 0 and type int), so the expression can be used
* e.g. in a structure initializer (or where-ever else comma expressions
* aren't permitted).
*/
#define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
#define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
/* &a[0] degrades to a pointer: a different type from an array */
#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
This gets used in various places to make sure we're dealing with an
array for a macro:
#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
So this builds:
struct untracked {
int size;
int array[];
} *a;
__must_be_array(a->array)
=> 0 (as expected)
__builtin_types_compatible_p(typeof(a->array), typeof(&(a->array)[0]))
=> 0 (as expected, array vs decayed array pointer)
But if counted_by is added, we get a build failure:
struct tracked {
int size;
int array[] __counted_by(size);
} *b;
__must_be_array(b->array)
=> build failure (not expected)
__builtin_types_compatible_p(typeof(b->array), typeof(&(b->array)[0]))
=> 1 (not expected, both pointers?)
--
Kees Cook
Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Mon, Jan 29, 2024 at 04:00:20PM +, Qing Zhao wrote:
> An update on the kernel building with my version 4 patch.
>
> Kees reported two FE issues with the current version 4 patch:
>
> 1. The operator “typeof” cannot return correct type for a->array;
> 2. The operator “&” cannot return correct address for a->array;
>
> I fixed both in my local repository.
>
> With these additional fix. Kernel with counted-by annotation can be built
> successfully.
Thanks for the fixes!
>
> And then, Kees reported one behavioral issue with the current counted-by:
>
> When the counted-by value is below zero, my current patch
>
> A. Didn’t report any warning for it.
> B. Accepted the negative value as a wrapped size.
>
> i.e. for:
>
> struct foo {
> signed char size;
> unsigned char array[] __counted_by(size);
> } *a;
>
> ...
> a->size = -3;
> report(__builtin_dynamic_object_size(p->array, 1));
>
> this reports 253, rather than 0.
>
> And the array-bounds sanitizer doesn’t catch negative index bounds neither.
>
> a->size = -3;
> report(a->array[1]); // does not trap
>
>
> So, my questions are:
>
> How should we handle the negative counted-by value?
Treat it as always 0-bounded: count < 0 ? 0 : count
>
> My approach is:
>
>I think that this is a user error, the compiler need to Issue warning
> during runtime about this user error.
>
> Since I have one remaining patch that has not been finished yet:
>
> 6 Emit warnings when the user breaks the requirments for the new counted_by
> attribute
> compilation time: -Wcounted-by
> run time: -fsanitizer=counted-by
> * The initialization to the size field should be done before the first
> reference to the FAM field.
I would hope that regular compile-time warnings would catch this.
> * the array has at least # of elements specified by the size field all
> the time during the program.
> * the value of counted-by should not be negative.
This seems reasonable for a very strict program, but it won't work for
the kernel as-is: a negative "count" is sometimes used to carry failure
details back to other users of the structure. This could be refactored in
the kernel, but I'd prefer that even without -fsanitizer=counted-by the
runtime behaviors will be "safe".
It does not seem sensible to me that adding a buffer size validation
primitive to GCC will result in conditions where a size calculation
will wrap around. I prefer no surprises. :)
> Let me know your comment and suggestions.
Clang has implemented the safety logic I'd prefer:
* __bdos will report 0 for any sizing where the "counted_by" count
variable is negative. Effectively, the count variable is always
processed as: count < 0 ? 0 : count
struct foo {
int count;
short array[] __counted_by(count);
} *p;
__bdos(p->array, 1) ==> sizeof(*p->array) * (count < 0 ? 0 : count)
The logic for this is that __bdos can be _certain_ that the size is 0
when the count variable is pathological.
* -fsanitize=array-bounds similarly treats count as above, so that:
printf("%d\n", p->array[index]); ==> trap when index > (count < 0 ? 0 : count)
Same logic for the sanitizer: any access to the array when count is
invalid means the access is invalid and must be trapped.
This means that software can run safely even in pathological conditions.
-Kees
--
Kees Cook
Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Mon, Jan 29, 2024 at 07:32:06PM +, Qing Zhao wrote:
>
>
> > On Jan 29, 2024, at 12:25 PM, Kees Cook wrote:
> >
> > On Mon, Jan 29, 2024 at 04:00:20PM +, Qing Zhao wrote:
> >> An update on the kernel building with my version 4 patch.
> >>
> >> Kees reported two FE issues with the current version 4 patch:
> >>
> >> 1. The operator “typeof” cannot return correct type for a->array;
> >> 2. The operator “&” cannot return correct address for a->array;
> >>
> >> I fixed both in my local repository.
> >>
> >> With these additional fix. Kernel with counted-by annotation can be built
> >> successfully.
> >
> > Thanks for the fixes!
> >
> >>
> >> And then, Kees reported one behavioral issue with the current counted-by:
> >>
> >> When the counted-by value is below zero, my current patch
> >>
> >> A. Didn’t report any warning for it.
> >> B. Accepted the negative value as a wrapped size.
> >>
> >> i.e. for:
> >>
> >> struct foo {
> >> signed char size;
> >> unsigned char array[] __counted_by(size);
> >> } *a;
> >>
> >> ...
> >> a->size = -3;
> >> report(__builtin_dynamic_object_size(p->array, 1));
> >>
> >> this reports 253, rather than 0.
> >>
> >> And the array-bounds sanitizer doesn’t catch negative index bounds
> >> neither.
> >>
> >> a->size = -3;
> >> report(a->array[1]); // does not trap
> >>
> >>
> >> So, my questions are:
> >>
> >> How should we handle the negative counted-by value?
> >
> > Treat it as always 0-bounded: count < 0 ? 0 : count
>
> Then the size of the object is 0?
That would be the purpose, yes. It's possible something else has
happened, but it would mean "the array contents should not be accessed
(since we don't have a valid size)".
>
> >
> >>
> >> My approach is:
> >>
> >> I think that this is a user error, the compiler need to Issue warning
> >> during runtime about this user error.
> >>
> >> Since I have one remaining patch that has not been finished yet:
> >>
> >> 6 Emit warnings when the user breaks the requirments for the new
> >> counted_by attribute
> >> compilation time: -Wcounted-by
> >> run time: -fsanitizer=counted-by
> >> * The initialization to the size field should be done before the first
> >> reference to the FAM field.
> >
> > I would hope that regular compile-time warnings would catch this.
> If the value is known at compile-time, then compile-time should catch it.
>
> >
> >> * the array has at least # of elements specified by the size field all
> >> the time during the program.
> >> * the value of counted-by should not be negative.
> >
> > This seems reasonable for a very strict program, but it won't work for
> > the kernel as-is: a negative "count" is sometimes used to carry failure
> > details back to other users of the structure. This could be refactored in
> > the kernel, but I'd prefer that even without -fsanitizer=counted-by the
> > runtime behaviors will be "safe".
>
> So, In the kernel’s source code, for example:
>
> struct foo {
> int count;
> short array[] __counted_by(count);
> };
>
> The field “count” will be used for two purposes:
> A. As the counted_by for the “array” when its value > 0;
> B. As an errno when its value < 0; under such condition, the size of “array”
> is zero.
>
> Is the understanding correct?
Yes.
> Is doing this for saving space? (Curious -:)
It seems so, yes.
> > It does not seem sensible to me that adding a buffer size validation
> > primitive to GCC will result in conditions where a size calculation
> > will wrap around. I prefer no surprises. :)
>
> Might be a bug here. I guess.
> >
> >> Let me know your comment and suggestions.
> >
> > Clang has implemented the safety logic I'd prefer:
> >
> > * __bdos will report 0 for any sizing where the "counted_by" count
> > variable is negative. Effectively, the count variable is always
> > processed as: count < 0 ? 0 : count
> >
> > struct foo {
> > int count;
> > short array[] __counted_by(count);
> > } *p;
> >
> > __bdos(p->array, 1) ==> sizeof(*p->array) * (count < 0 ? 0 : c
Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Mon, Jan 29, 2024 at 10:45:23PM +, Qing Zhao wrote:
> There are two things here.
>
> 1. The value of the “counted-by” is 0; (which is easy to be understood)
> 2. The result of the _builtin_object_size when see a “counted-by” 0.
>
> For 1, it’s simple, if we see a counted-by value <= 0, then counted-by is 0;
Okay, that's good; this matches my understanding. :)
> But for 2, when the _builtin_object_size sees a “counted-by” 0, what’s value
> it will return for the object size?
>
> Can we return 0 for the object size?
I don't see why not. For example:
// -O2 -fstrict-flex-arrays=3
struct s {
int a;
int b[4];
} foo;
#define report(x) printf("%s: %zu\n", #x, (size_t)(x))
int main(int argc, char *argv[])
{
struct s foo;
report(__builtin_dynamic_object_size(&foo.b[4], 0));
report(__builtin_dynamic_object_size(&foo.b[5], 0));
report(__builtin_dynamic_object_size(&foo.b[-10], 0));
report(__builtin_dynamic_object_size(&foo.b[4], 1));
report(__builtin_dynamic_object_size(&foo.b[5], 1));
report(__builtin_dynamic_object_size(&foo.b[-10], 1));
report(__builtin_dynamic_object_size(&foo.b[4], 2));
report(__builtin_dynamic_object_size(&foo.b[5], 2));
report(__builtin_dynamic_object_size(&foo.b[-10], 2));
report(__builtin_dynamic_object_size(&foo.b[4], 3));
report(__builtin_dynamic_object_size(&foo.b[5], 3));
report(__builtin_dynamic_object_size(&foo.b[-10], 3));
return 0;
}
shows:
__builtin_dynamic_object_size(&foo.b[4], 0): 0
__builtin_dynamic_object_size(&foo.b[5], 0): 0
__builtin_dynamic_object_size(&foo.b[-10], 0): 0
__builtin_dynamic_object_size(&foo.b[4], 1): 0
__builtin_dynamic_object_size(&foo.b[5], 1): 0
__builtin_dynamic_object_size(&foo.b[-10], 1): 0
__builtin_dynamic_object_size(&foo.b[4], 2): 0
__builtin_dynamic_object_size(&foo.b[5], 2): 0
__builtin_dynamic_object_size(&foo.b[-10], 2): 0
__builtin_dynamic_object_size(&foo.b[4], 3): 0
__builtin_dynamic_object_size(&foo.b[5], 3): 0
__builtin_dynamic_object_size(&foo.b[-10], 3): 0
This is showing "no bytes left" for the end of the b array, and if this
index keeps going, it still reports 0 if we're past the end of the object
completely. And it is similarly capped for negative indexes. This is
true for all the __bos type bits.
A "counted-by" of 0 (or below) would have the same meaning as an out of
bounds index here.
> (As I mentioned in the previous email, 0 in __builtin_object_size doesn’t
> mean size 0,
> it means UNKNOWN_SIZE when the type is 2/3, So, what’s value we should
> return for the size 0?)
> https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html
I think I see what you mean, but I still think it should be 0 for 2/3,
regardless of the documented interpretation. If that's the current
response for a pathological index under 2/3, then I think it's totally
reasonable that it should do the same for pathological bounds.
And BTW, it seems there are 0-sized objects, though maybe they're some
kind of special case:
struct s {
int a;
struct { } nothing;
int b;
};
#define report(x) printf("%s: %zu\n", #x, (size_t)(x))
int main(int argc, char *argv[])
{
struct s foo;
report(__builtin_dynamic_object_size(&foo.nothing, 1));
}
shows:
__builtin_dynamic_object_size(&foo.nothing, 1): 0
-Kees
--
Kees Cook
Re: [RFC/RFT,V2] CFI: Add support for gcc CFI in aarch64
On Wed, Dec 13, 2023 at 05:01:07PM +0800, Wang wrote: > On 2023/12/13 16:48, Dan Li wrote: > > + Likun > > > > On Tue, 28 Mar 2023 at 06:18, Sami Tolvanen wrote: > >> On Mon, Mar 27, 2023 at 2:30 AM Peter Zijlstra > >> wrote: > >>> On Sat, Mar 25, 2023 at 01:54:16AM -0700, Dan Li wrote: > >>> > >>>> In the compiler part[4], most of the content is the same as Sami's > >>>> implementation[3], except for some minor differences, mainly including: > >>>> > >>>> 1. The function typeid is calculated differently and it is difficult > >>>> to be consistent. > >>> This means there is an effective ABI break between the compilers, which > >>> is sad :-( Is there really nothing to be done about this? > >> I agree, this would be unfortunate, and would also be a compatibility > >> issue with rustc where there's ongoing work to support > >> clang-compatible CFI type hashes: > >> > >> https://github.com/rust-lang/rust/pull/105452 > >> > >> Sami > > > Hi Peter and Sami > > I am Dan Li's colleague, and I will take over and continue the work of CFI. Welcome; this is great news! :) Thanks for picking up the work. > > Regarding the issue of gcc cfi type id being compatible with clang, we > have analyzed and verified: > > 1. clang uses Mangling defined in Itanium C++ ABI to encode the function > prototype, and uses the encoding result as input to generate cfi type id; > 2. Currently, gcc only implements mangling for the C++ compiler, and the > function prototype coding generated by these interfaces is compatible > with clang, but gcc's c compiler does not support mangling.; > > Adding mangling to gcc's c compiler is a huge and difficult task，because > we have to refactor the mangling of C++, splitting it into basic > mangling and language specific mangling, and adding support for the c > language which requires a deep understanding of the compiler and > language processing parts. > > And for the kernel cfi, I suggest separating type compatibility from CFI > basic functions. Type compatibility is independent from CFI basic > funcitons and should be dealt with under another topic. Should we focus > on the main issus of cfi, and let it work first on linux kernel, and > left the compatible issue to be solved later? If you mean keeping the hashes identical between Clang/LLVM and GCC, I think this is going to be a requirement due to adding Rust to the build environment (which uses the LLVM mangling and hashing). FWIW, I think the subset of type mangling needed isn't the entirely C++ language spec, so it shouldn't be hard to add this to GCC. -Kees -- Kees Cook
Re: C runtime checking for assigment of VM types, v3
On Mon, Jul 15, 2024 at 09:19:49AM +0200, Martin Uecker wrote:
> The instrumentation is guarded by a new instrumentation flag -fvla-bounds,
> but runtime overhead should generally be very low as most checks are
> removed by the optimizer, e.g.
>
> void foo(int x, char (*buf)[x])
> {
> bar(x, buf);
> }
>
> does not have any overhead with -O1 (we also might want to filter out
> some obvious cases already in the FE). So I think this flag could be
> a good addition to -fhardened after some testing. Maybe it could even
> be activated by default.
Just to clarify, but does any of this change the behavior of
__builtin_object_size() or __builtin_dynamic_object_size() within
functions that take array arguments?
i.e. does this work now?
void foo(int array[10])
{
global = __builtin_object_size(array, 1);
}
(Currently "global" will be set to SIZE_MAX, rather than 40.)
--
Kees Cook
Re: C runtime checking for assigment of VM types, v3
On Mon, Jul 15, 2024 at 07:20:31PM +0200, Martin Uecker wrote:
> No, there are still two many missing pieces. The following
> works already
>
> int h(int n, int buf[n])
> {
> return __builtin_dynamic_object_size(buf, 1);
> }
Yeah, this is nice.
There are some interesting things happening around this general
idea. Clang has the rather limited attributes "pass_object_size" and
"pass_dynamic_object_size" that will work on function prototypes that
will inform a _bos or _bdos internally, but you can only choose _one_
type to apply to a given function parameter:
size_t h(char * const __attribute__((pass_dynamic_object_size(1))) buf)
{
return __builtin_dynamic_object_size(buf, 1);
}
https://clang.llvm.org/docs/AttributeReference.html#pass-object-size-pass-dynamic-object-size
I have found it easier to just make wrapper macros, as that can allow
both size types:
size_t h(char *buf, const size_t p_max, const size_t p_min);
#define h(p)\
({ \
const size_t __p_max = __builtin_dynamic_object_size(p, 0); \
const size_t __p_min = __builtin_dynamic_object_size(p, 1); \
__h(p, __p_max, __p_min); \
})
But best is that it just gets handled automatically, which will be the
goals of the more generalized "counted_by" (and "sized_by") attributes
that will provide similar coverage as your example:
size_t h(int * __attribute__((sized_by(bytes))) buf, int bytes)
{
return __builtin_dynamic_object_size(buf, 1);
}
https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/
Those attributes end up being similar to what you have only the explicit
predeclaration isn't needed. i.e. to put "int n" in your example after
"buf", it needs predeclaration:
int h(int n; int buf[n], int n)
{
...
}
(But Clang doesn't appear to support predeclarations.)
--
Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Mon, May 13, 2024 at 07:48:30PM +, Qing Zhao wrote: > The false positive warnings are moved from -Warray-bounds=1 to > -Warray-bounds=2 now. On a Linux kernel x86_64 allmodconfig build, this takes the -Warray-bounds warnings from 14 to 9. After examining these 9, I see: - 4: legitimate bugs in Linux source (3 distinct, 1 repeat). I'll be reporting/fixing these in Linux. - 4: 4 instances of 1 case of buggy interaction with -fsanitize=shift, looking similar to these past bugs: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108306 the difference being operating on an enum. I will reduce the case and open a bug report for it. - 1: still examining. It looks like a false positive so far. Thanks! -Kees -- Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Tue, May 14, 2024 at 02:17:16PM +, Qing Zhao wrote:
> The current major issue with the warning is: the constant index value 4
> is not in the source code, it’s a compiler generated intermediate value
> (even though it’s a correct value -:)). Such warning messages confuse
> the end-users with information that cannot be connected directly to the
> source code.
Right -- this "4" comes from -fsanitize=array-bounds (in "warn but
continue" mode).
Now, the minimized PoC shows a situation that triggers the situation, but
I think it's worth looking at the original code that caused this false
positive:
for (i = 0; i < sg->num_entries; i++) {
gce = &sg->gce[i];
The issue here is that sg->num_entries has already been bounds-checked
in a separate function. As a result, the value range tracking for "i"
here is unbounded.
Enabling -fsanitize=array-bounds means the sg->gce[i] access gets
instrumented, and suddenly "i" gains an implicit range, induced by the
sanitizer.
(I would point out that this is very similar to the problems we've had
with -fsanitize=shift[1][2]: the sanitizer induces a belief about a
given variable's range this isn't true.)
Now, there is an argument to be made that the original code should be
doing:
for (i = 0; i < 4 && i < sg->num_entries; i++) {
But this is:
a) logically redundant (Linux maintainers don't tend to like duplicating
their range checking)
b) a very simple case
The point of the sanitizers is to catch "impossible" situations at
run-time for the cases where some value may end up out of range. Having
it _induce_ a range on the resulting code makes no sense.
Could we, perhaps, have sanitizer code not influence the value range
tracking? That continues to look like the root cause for these things.
-Kees
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105679
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108306
--
Kees Cook
Re: [PATCH v1] Provide new GCC builtin __builtin_get_counted_by [PR116016]
On Tue, Aug 13, 2024 at 03:33:26PM +, Qing Zhao wrote: > With the addition of the 'counted_by' attribute and its wide roll-out > within the Linux kernel, a use case has been found that would be very > nice to have for object allocators: being able to set the counted_by > counter variable without knowing its name. This tests great for me; thanks! My prototype allocator example I used for testing is here: https://github.com/kees/kernel-tools/blob/trunk/fortify/get_counted_by.c -- Kees Cook
Re: [PATCH v1] Provide new GCC builtin __builtin_get_counted_by [PR116016]
On Wed, Aug 21, 2024 at 03:27:56PM +, Qing Zhao wrote:
> > On Aug 21, 2024, at 10:45, Martin Uecker wrote:
> >
> > Am Mittwoch, dem 21.08.2024 um 16:34 +0200 schrieb Martin Uecker:
> >> Am Mittwoch, dem 21.08.2024 um 14:12 + schrieb Qing Zhao:
> >>
> >>>
> >>> Yes, I do feel that the approach __builtin_get_counted_by is not very
> >>> good.
> >>> Maybe it’s better to provide
> >>> A. __builtin_set_counted_by
> >>> or
> >>> B. The unary operator __counted_by(PTR) to return a Lvalue, in this case,
> >>> we need a __builtin_has_attribute first to check whether PTR has the
> >>> counted_by attribute first.
> >>
> >> You could potentially do the same __counted_by and test for type void.
> >>
> >> _Generic(typeof(__counted_by(PTR)), void: (void)0, __counted_by(PTR) =
> >> COUNT);
> >
> > But just doing A. also seems ok.
>
> I am fine with A. It’s easier to be used by the end users.
>
> The only potential problem with A is, the functionality of READing the
> counted-by field is missing.
> Is that okay? Kees?
After seeing the utility of __builtin_get_counted_by() I realized that
we really do want it for the ability to examine the _type_ of the
counter member, otherwise we run the risk of assignment truncation. For
example:
struct flex {
unsigned char counter;
int array[] __attribute__((counted_by(counter)));
} *p;
count = 1000;
...
__builtin_set_counted_by(p->array, count);
What value would p->counter end up with? (I assume it would wrap around,
which is bad). And there would be no way to catch it at run-time without
a way to check the type. For example with __builtin_get_counted_by, we
can do:
if (__builtin_get_counted_by(p->array)) {
size_t max_value =
type_max(typeof(*__builtin_get_counted_by(p->array)));
if (count > type_max)
...fail cleanly...
*__builtin_get_counted_by(p->array) = count;
}
I don't strictly need to READ the value (but it seems nice). Currently I can
already do a READ with something like this:
size_t count = __builtin_dynamic_object_size(p->array, 1) / sizeof(*p->array);
But I don't have a way to examine the counter _type_ without
__builtin_get_counted_by, so I prefer it over __builtin_set_counted_by.
Thanks!
-Kees
--
Kees Cook
Re: [PATCH v1] Provide new GCC builtin __builtin_get_counted_by [PR116016]
On Wed, Aug 21, 2024 at 05:43:42PM +0200, Martin Uecker wrote: > Am Mittwoch, dem 21.08.2024 um 15:24 + schrieb Qing Zhao: > > > > > > But if we changed it to return a void pointer, we could make this > > > a compile-time check: > > > > > > auto ret = __builtin_get_counted_by(__p->FAM); > > > > > > _Generic(ret, void*: (void)0, default: *ret = COUNT); > > > > Is there any benefit to return a void pointer than a SIZE_T pointer for > > the NULL pointer? > > Yes! You can test with _Generic (or __builtin_types_compatible_p) > at compile-time based on the type whether you can set *ret to COUNT > or not as in the example above. > > So it is not a weird run-time test which needs to be optimized > away. I don't have a strong opinion here, but I would tend to agree that returning "void *" is a better signal that it is not valid. And I do really like the _Generic example there, which makes it even easier to do the "set if counted_by" action. -- Kees Cook
Re: [PATCH v1] Provide new GCC builtin __builtin_get_counted_by [PR116016]
On Mon, Aug 26, 2024 at 07:30:15PM +, Qing Zhao wrote:
> Hi, Martin,
>
> Looks like that there is some issue when I tried to use the _Generic for the
> testing cases, and then I narrowed down to a
> small testing case that shows the problem without any change to GCC.
>
> [opc@qinzhao-ol8u3-x86 gcc]$ cat t1.c
> struct annotated {
> char b;
> int c[];
> } *array_annotated;
> extern void * counted_by_ref (int *);
>
> int main(int argc, char *argv[])
> {
> typeof(counted_by_ref (array_annotated->c)) ret
> = counted_by_ref (array_annotated->c);
>_Generic (ret, void* : (void)0, default: *ret = 10);
>
> return 0;
> }
> [opc@qinzhao-ol8u3-x86 gcc]$ /home/opc/Install/latest/bin/gcc t1.c
> t1.c: In function ‘main’:
> t1.c:12:44: warning: dereferencing ‘void *’ pointer
>12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> |^~~~
> t1.c:12:49: error: invalid use of void expression
>12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> | ^
I implemented it like this[1] in the Linux kernel. So yours could be:
struct annotated {
char b;
int c[] __attribute__((counted_by(b));
};
extern struct annotated *array_annotated;
int main(int argc, char *argv[])
{
typeof(_Generic(__builtin_get_counted_by(array_annotated->c),
void *: (size_t *)NULL,
default: __builtin_get_counted_by(array_annotated->c)))
ret = __builtin_get_counted_by(array_annotated->c);
if (ret)
*ret = 10;
return 0;
}
It's a bit cumbersome, but it does what's needed.
This is, however, just doing exactly what Bill has suggested: it is
converting the (void *)NULL into (size_t *)NULL when there is no
counted_by annotation...
-Kees
[1]
https://lore.kernel.org/linux-hardening/20240822231324.make.666-k...@kernel.org/
--
Kees Cook
Re: [PATCH v1] Provide new GCC builtin __builtin_get_counted_by [PR116016]
On Mon, Aug 26, 2024 at 11:01:08PM +0200, Martin Uecker wrote:
> Am Montag, dem 26.08.2024 um 13:30 -0700 schrieb Kees Cook:
> > On Mon, Aug 26, 2024 at 07:30:15PM +, Qing Zhao wrote:
> > > Hi, Martin,
> > >
> > > Looks like that there is some issue when I tried to use the _Generic for
> > > the testing cases, and then I narrowed down to a
> > > small testing case that shows the problem without any change to GCC.
> > >
> > > [opc@qinzhao-ol8u3-x86 gcc]$ cat t1.c
> > > struct annotated {
> > > char b;
> > > int c[];
> > > } *array_annotated;
> > > extern void * counted_by_ref (int *);
> > >
> > > int main(int argc, char *argv[])
> > > {
> > > typeof(counted_by_ref (array_annotated->c)) ret
> > > = counted_by_ref (array_annotated->c);
> > >_Generic (ret, void* : (void)0, default: *ret = 10);
> > >
> > > return 0;
> > > }
> > > [opc@qinzhao-ol8u3-x86 gcc]$ /home/opc/Install/latest/bin/gcc t1.c
> > > t1.c: In function ‘main’:
> > > t1.c:12:44: warning: dereferencing ‘void *’ pointer
> > >12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> > > |^~~~
> > > t1.c:12:49: error: invalid use of void expression
> > >12 | _Generic (ret, void* : (void)0, default: *ret = 10);
> > > | ^
> >
> > I implemented it like this[1] in the Linux kernel. So yours could be:
> >
> > struct annotated {
> > char b;
> > int c[] __attribute__((counted_by(b));
> > };
> > extern struct annotated *array_annotated;
> >
> > int main(int argc, char *argv[])
> > {
> > typeof(_Generic(__builtin_get_counted_by(array_annotated->c),
> >void *: (size_t *)NULL,
> >default: __builtin_get_counted_by(array_annotated->c)))
> > ret = __builtin_get_counted_by(array_annotated->c);
> > if (ret)
> > *ret = 10;
> >
> > return 0;
> > }
> >
> > It's a bit cumbersome, but it does what's needed.
> >
> > This is, however, just doing exactly what Bill has suggested: it is
> > converting the (void *)NULL into (size_t *)NULL when there is no
> > counted_by annotation...
> >
> > -Kees
> >
> > [1]
> > https://lore.kernel.org/linux-hardening/20240822231324.make.666-k...@kernel.org/
>
> Interesting. Will __builtin_get_counted_by(array_annotated->c) give
> a null pointer (or an invalid pointer) of the correct type if
> array_annotated is a null pointer of an annotated struct type?
If you mean this part:
typeof(P) __obj_ptr = NULL; \
/* Just query the counter type for type_max checking. */ \
typeof(_Generic(__flex_counter(__obj_ptr->FAM), \
void *: (size_t *)NULL, \
default: __flex_counter(__obj_ptr->FAM))) \
__counter_type_ptr = NULL; \
Where __obj_ptr starts as NULL, then yes. (Or at least, yes it does
currently with Qing's GCC patch and Bill's Clang patch.)
> I also wonder a bit about the multiple macro evaluations of the arguments
> for P and SIZE.
I tried to design it so they aren't used with anything that should
have side-effects.
Anyway, if __builtin_get_counted_by returns (size_t *)NULL then I think
the _Generic wrapping isn't needed. That would make it easier to use?
-Kees
--
Kees Cook
Re: [RFC][PATCH v1 0/4] Allow flexible array members in unions and alone in structures [PR53548]
On Fri, Apr 19, 2024 at 06:43:13PM +, Qing Zhao wrote: > Therefore, GCC needs to explicitly allow such extensions directly for C99 > flexible arrays, since flexable array member in unions or alone in structs > are common code patterns in active use by the Linux kernel (and other > projects). Thank you for fixing this! :) This will make conversions much much easier for the Linux kernel (and future userspace programs). I've tested these patches and everything behaves like I'd expect. -Kees -- Kees Cook
Re: [PATCH v3 1/4] Allow flexible array members in unions and alone in structures [PR53548]
On Tue, Apr 30, 2024 at 05:51:20PM -0400, Jason Merrill wrote:
> On 4/30/24 14:45, Qing Zhao wrote:
> >
> >
> > > On Apr 30, 2024, at 15:27, Jason Merrill wrote:
> > >
> > > On 4/30/24 07:58, Qing Zhao wrote:
> > > > The request for GCC to accept that the C99 flexible array member can be
> > > > in a union or alone in a structure has been made a long time ago
> > > > around 2012
> > > > for supporting several practical cases including glibc.
> > > > A GCC PR has been opened for such request at that time:
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53548
> > > > However, this PR was closed as WONTFIX around 2015 due to the
> > > > following reason:
> > > > "there is an existing extension that makes the requested
> > > > functionality possible"
> > > > i.e GCC fully supported that the zero-length array can be in a
> > > > union or alone
> > > > in a structure for a long time. (though I didn't see any
> > > > official documentation
> > > > on such extension)
> > > > It's reasonable to close PR53548 at that time since zero-length
> > > > array extension
> > > > can be used for such purpose.
> > > > However, since GCC13, in order to improve the C/C++ security, we
> > > > introduced
> > > > -fstrict-flex-arrays=n to gradually eliminate the "fake flexible array"
> > > > usages from C/C++ source code. As a result, zero-length arrays
> > > > eventually
> > > > will be replaced by C99 flexiable array member completely.
> > > > Therefore, GCC needs to explicitly allow such extensions directly for
> > > > C99
> > > > flexible arrays, since flexable array member in unions or alone
> > > > in structs
> > > > are common code patterns in active use by the Linux kernel (and
> > > > other projects).
> > > > For example, these do not error by default with GCC:
> > > > union one {
> > > > int a;
> > > > int b[0];
> > > > };
> > > > union two {
> > > > int a;
> > > > struct {
> > > > struct { } __empty;
> > > > int b[];
> > > > };
> > > > };
> > > > But these do:
> > > > union three {
> > > > int a;
> > > > int b[];
> > > > };
> > > > struct four {
> > > > int b[];
> > > > }
> > > > Clang has supported such extensions since March, 2024
> > > > https://github.com/llvm/llvm-project/pull/84428
> > > > GCC should also support such extensions. This will allow for
> > > > a seamless transition for code bases away from zero-length arrays
> > > > without
> > > > losing existing code patterns.
> > > > gcc/ChangeLog:
> > > > * doc/extend.texi: Add documentation for Flexible Array Members in
> > > > Unions and Flexible Array Members alone in Structures.
> > > > ---
> > > > gcc/doc/extend.texi | 34 ++
> > > > 1 file changed, 34 insertions(+)
> > > > diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> > > > index 7b54a241a7bf..cba98c8aadd7 100644
> > > > --- a/gcc/doc/extend.texi
> > > > +++ b/gcc/doc/extend.texi
> > > > @@ -42,6 +42,8 @@ extensions, accepted by GCC in C90 mode and in C++.
> > > > * Named Address Spaces::Named address spaces.
> > > > * Zero Length:: Zero-length arrays.
> > > > * Empty Structures:: Structures with no members.
> > > > +* Flexible Array Members in Unions:: Unions with Flexible
> > > > Array Members.
> > > > +* Flexible Array Members alone in Structures:: Structures with
> > > > only Flexible Array Members.
> > > > * Variable Length:: Arrays whose length is computed at run time.
> > > > * Variadic Macros:: Macros with a variable number of arguments.
> > > > * Escaped Newlines:: Slightly looser rules for escaped newlines.
> > > > @@ -1873,6 +1875,38 @@ The structure has size zero. In C++,
> > > > empty structures are part
> > > > of the language. G++ treats empty structures as if they had a single
> > > > member of type @code{char}.
> > > > +@node Flexible Array Members in Unions
> > > > +@section Unions with Flexible Array Members
> > > > +@cindex unions with flexible array members
> > > > +@cindex unions with FAMs
> > > > +
> > > > +GCC permits a C99 flexible array member (FAM) to be in a union:
> > > > +
> > > > +@smallexample
> > > > +union with_fam @{
> > > > + int a;
> > > > + int b[];
> > > > +@};
> > > > +@end smallexample
> > > > +
> > > > +If all the members of a union are flexible array member, the size of
> >
> > It’s for the following case:
> >
> > union with_fam_3 {
> > char a[];
> > int b[];
> > }
> >
> > And also include: (the only member of a union is a flexible array
> > member as you mentioned below)
> >
> > union with_fam_1 {
> > char a[];
> > }
> >
> > So, I think the original sentence:
> >
> > “If all the members of a union are flexible array member, the size of”
> >
> > Should be better than the below:
> > >
> > > "If the only member of a union is a flexible array member”
>
> Makes sense, but then it should be "members" both times rather than
> "members" and then "member".
"If every member of a union is a flexible array, the size ..." ?
--
Kees Cook
Re: [V2][PATCH] gcc-14/changes.html: Deprecate a GCC C extension on flexible array members.
d_Configured_control {
Thread_Control Control;
#if CONFIGURE_MAXIMUM_USER_EXTENSIONS > 0
void *extensions[ CONFIGURE_MAXIMUM_USER_EXTENSIONS + 1 ];
#endif
Configuration_Scheduler_node Scheduler_nodes[ _CONFIGURE_SCHEDULER_COUNT ];
RTEMS_API_Control API_RTEMS;
#ifdef RTEMS_POSIX_API
POSIX_API_Control API_POSIX;
#endif
#if CONFIGURE_MAXIMUM_THREAD_NAME_SIZE > 1
char name[ CONFIGURE_MAXIMUM_THREAD_NAME_SIZE ];
#endif
#if defined(_CONFIGURE_ENABLE_NEWLIB_REENTRANCY) && \
!defined(_REENT_THREAD_LOCAL)
struct _reent Newlib;
#endif
};
#define THREAD_INFORMATION_DEFINE( name, api, cls, max ) \
...
static ... \
Thread_Configured_control \
name##_Objects[ _Objects_Maximum_per_allocation( max ) ]; \
...
I don't see any static initialization of struct _Thread_Control::extensions
nor any member initialization of the name##_Objects, and even then that
is all legal in any arrangement:
truct flex { int length; char data[]; };
struct mid_flex { int m; struct flex flex_data; int n; int o; };
struct end_flex { int m; int n; struct flex flex_data; };
struct flex f = { .length = 2 };
struct mid_flex m = { .m = 5 };
struct end_flex e = { .m = 5 };
struct flex fa[4] = { { .length = 2 } };
struct mid_flex ma[4] = { { .m = 5 } };
struct end_flex ea[4] = { { .m = 5 } };
These all work.
But yes, I see why you can't move Thread_Control trivially to the end. It
looks like you're depending on the implicit overlapping memory locations
between struct _Thread_Control and things that include it as the first
struct member, like struct Thread_Configured_control above:
cpukit/score/src/threaditerate.c: the_thread = (Thread_Control *)
information->local_table[ index ];
(In the Linux kernel we found this kind of open casting to be very
fragile and instead use a horrific wrapper called "container_of"[3] that
does the pointer math (possibly to an offset of 0 for a direct cast) to
find the member.)
Anyway, for avoiding the warning, you can just keep using the extension
and add -Wno-... if it ever ends up in -Wall, or you can redefine struct
_Thread_Control to avoid having the "extensions" member at all. This is
what we've done in several cases in Linux. For example if we had this
again, but made to look more like Thread_Control:
struct flex { int foo; int bar; char data[]; };
struct mid_flex { struct flex hdr; int n; int o; };
It could be changed to:
struct flex_hdr { int foo; int bar; };
struct flex { struct flex_hdr hdr; char data[]; };
struct mid_flex { struct flex_hdr hdr; int n; int o; };
This has some collateral changes needed to reference the struct flex_hdr
members from struct flex now (f->hdr.foo instead of f->foo). Sometimes
this can be avoided by using a union, as I did in a recent refactoring
in Linux: [4]
For more complex cases in Linux we've handled this by using our
"struct_group"[5] macro, which allows for a union and tagged struct to
be constructed:
struct flex {
__struct_group(flex_hdr, hdr,,
int foo;
int bar;
);
char data[];
};
struct mid_flex { struct flex_hdr hdr; int n; int o; };
Then struct flex member names don't have to change, but if anything is
trying to get at struct flex::data through struct mid_flex::hdr, that'll
need casting. But it _shouldn't_ since it has "n" and "o".
-Kees
[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-May/620122.html
[2] https://github.com/RTEMS/rtems
[3]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/container_of.h#n10
[4] https://git.kernel.org/linus/896880ff30866f386ebed14ab81ce1ad3710cfc4
[5]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/stddef.h?h=v6.8#n11
--
Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
ex];" where the value range tracking for "index" is now [4,INT_MAX], is the warning valid. But it is only for that instance. Reporting it for effectively both (there is only 1 source line for the array indexing) is misleading because there is nothing the user can do about it -- the compiler created the copy and then noticed it had a range it could apply to that array index. This situation makes -Warray-bounds unusable for the Linux kernel (we cannot have false positives says BDFL), but we'd *really* like to have it enabled since it usually finds real bugs. But these false positives can't be fixed on our end. :( So, moving them to -Warray-bounds=2 makes sense as that's the level documented to have potential false positives. -Kees -- Kees Cook
Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading
On Tue, May 14, 2024 at 01:38:49AM +0200, Andrew Pinski wrote:
> On Mon, May 13, 2024, 11:41 PM Kees Cook wrote:
> > But it makes no sense to warn about:
> >
> > void sparx5_set (int * ptr, struct nums * sg, int index)
> > {
> >if (index >= 4)
> > warn ();
> >*ptr = 0;
> >*val = sg->vals[index];
> >if (index >= 4)
> > warn ();
> >*ptr = *val;
> > }
> >
> > Because at "*val = sg->vals[index];" the actual value range tracking for
> > index is _still_ [INT_MIN,INT_MAX]. (Only within the "then" side of the
> > "if" statements is the range tracking [4,INT_MAX].)
> >
> > However, in the case where jump threading has split the execution flow
> > and produced a copy of "*val = sg->vals[index];" where the value range
> > tracking for "index" is now [4,INT_MAX], is the warning valid. But it
> > is only for that instance. Reporting it for effectively both (there is
> > only 1 source line for the array indexing) is misleading because there
> > is nothing the user can do about it -- the compiler created the copy and
> > then noticed it had a range it could apply to that array index.
> >
>
> "there is nothing the user can do about it" is very much false. They could
> change warn call into a noreturn function call instead. (In the case of
> the Linux kernel panic). There are things the user can do to fix the
> warning and even get better code generation out of the compilers.
This isn't about warn() not being noreturn. The warn() could be any
function call; the jump threading still happens.
GCC is warning about a compiler-constructed situation that cannot be
reliably fixed on the source side (GCC emitting the warning is highly
unstable in these cases), since the condition is not *always* true for
the given line of code. If it is not useful to warn for "array[index]"
being out of range when "index" is always [INT_MIN,INT_MAX], then it
is not useful to warn when "index" MAY be [INT_MIN,INT_MAX] for a given
line of code.
-Kees
--
Kees Cook
Re: status of -fstack-protector-strong?
Han, Have you done full testsuite runs with and without this patch? I know you mentioned you've done builds on both x86_64 and arm; it might be nice to compare tests too. Thanks, -Kees On Mon, Sep 10, 2012 at 5:26 PM, Matthias Klose wrote: > On 08.09.2012 01:07, Kees Cook wrote: >> Hi, >> >> I'm curious about the status of this patch: >> http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00974.html >> >> Chrome OS uses this, and the Ubuntu Security Team has expressed >> interest in it as well. What's needed to land this in gcc? > > I don't see any statement about testsuite results and possible regressions > with > this patch (not only on x86 platforms). > > Matthias > -- Kees Cook Chrome OS Security
status of -fstack-protector-strong?
Hi, I'm curious about the status of this patch: http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00974.html Chrome OS uses this, and the Ubuntu Security Team has expressed interest in it as well. What's needed to land this in gcc? Thanks! -Kees -- Kees Cook Chrome OS Security
Re: Should -fsanitize=bounds support counted-by attribute for pointers inside a structure?
On Tue, Nov 19, 2024 at 05:41:13PM +0100, Martin Uecker wrote:
> Am Dienstag, dem 19.11.2024 um 10:47 -0500 schrieb Marek Polacek:
> > On Mon, Nov 18, 2024 at 07:10:35PM +0100, Martin Uecker wrote:
> > > Am Montag, dem 18.11.2024 um 17:55 + schrieb Qing Zhao:
> > > > Hi,
> > > >
> > > > I am working on extending “counted_by” attribute to pointers inside a
> > > > structure per our previous discussion.
> > > >
> > > > I need advice on the following question:
> > > >
> > > > Should -fsantize=bounds support array reference that was referenced
> > > > through a pointer that has counted_by attribute?
> >
> > I don't see why it couldn't, perhaps as part of -fsanitize=bounds-strict.
> > Someone has to implement it, though.
>
> I think Qing was volunteering to do this. My point was that
> this would not necessarily be undefined behavior, but instead
> could trap for possibly defined behavior. I would not mind, but
> I point out that in the past people insisted that the sanitizers
> are only intended to screen for undefined behavior.
I think it's a mistake to confuse the sanitizers with only addressing
"undefined behavior". The UB sanitizers are just a subset of the
sanitizers in general, and I think UB is a not a good category for how
to group the behaviors.
For the Linux kernel, we want robustness. UB leads to ambiguity, so
we're quite interested in getting rid of UB, but the bounds sanitizer is
expected to implement bounds checking, regardless of UB-ness.
I would absolutely want -fsanitize=bounds to check the construct Qing
mentioned.
Another aspect I want to capture for Linux is _pointer_ bounds, so that
this would be caught:
#include
struct annotated {
int b;
int *c __attribute__ ((counted_by (b)));
} *p_array_annotated;
void __attribute__((__noinline__)) setup (int annotated_count)
{
p_array_annotated
= (struct annotated *)malloc (sizeof (struct annotated));
p_array_annotated->c = (int *) malloc (annotated_count * sizeof (int));
p_array_annotated->b = annotated_count;
return;
}
int main(int argc, char *argv[])
{
int i;
int *c;
setup (10);
c = p_array_annotated->c;
for (i = 0; i < 11; i++)
*c++ = 2; // goes boom at i == 10
return 0;
}
This may be a separate sanitizer, and it may require a totally different
set of internal tracking, but being able to discover that we've run off
the end of an allocation is needed.
Of course, the biggest deal is that
__builtin_dynamic_object_size(p_array_annotated->c, 1) will return
10 * sizeof(*p_array_annotated->c)
>
> >
> > > I think the question is what -fsanitize=bounds is meant to be.
> > >
> > > I am a bit frustrated about the sanitizer. On the
> > > one hand, it is not doing enough to get spatial memory
> > > safety even where this would be easily possible, on the
> > > other hand, is pedantic about things which are technically
> > > UB but not problematic and then one is prevented from
> > > using it
> > >
> > > When used in default mode, where execution continues, it
> > > also does not mix well with many warning, creates more code,
> > > and pulls in a libary dependency (and the library also depends
> > > on upstream choices / progress which seems a limitation for
> > > extensions).
> > >
> > > What IMHO would be ideal is a protection mode for spatial
> > > memory safety that simply adds traps (which then requires
> > > no library, has no issues with other warnings, and could
> > > evolve independently from clang)
> > >
> > > So shouldn't we just add a -fboundscheck (which would
> > > be like -fsanitize=bounds -fsanitize-trap=bounds just with
> > > more checking) and make it really good? I think many people
> > > would be very happy about this.
> >
> > That's a separate concern. We already have the -fbounds-check option,
> > currently only used in Fortran (and D?), so perhaps we could make
> > that option a shorthand for -fsanitize=bounds -fsanitize-trap=bounds.
>
> I think it could share large parts of the implementation, but the
> main reason for having a separate option would be to do something
> better than the sanitizer. So it could not simply be a shorthand.
I don't want to reinvent the wheel here -- the sanitizers already have 3
modes of operation (trap, callback with details, callback without
details), and Linux uses the first 2 modes already, and has had plans to
use the third (smaller resulting image).
Most notably, Linux _must_ have a warn-only mode or the feature will
never get merged (this is a hard requirement from Linus). All serious
deployments of the feature will use either trap mode or use the
trap-on-warn setting, of course. But for the feature to even see the
light of day, Linus requires there be a warn-only mode.
So, given these requirements, continuing to use the sanitizer framework
seems much simpler to me. :)
-Kees
--
Kees Cook
Re: [PATCH v4 0/3][RFC]Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
On Tue, Nov 05, 2024 at 04:31:29PM +, Qing Zhao wrote: > This is the 4th version of the patch for fixing PR109071. Thanks for all the work on this! I've been doing Linux kernel builds with it and am finding many places where the new diagnostics make the warning understandable. There was an especially hard to understand one that even with diagnostics took me some time to track down. :) Here is an example of one of the fixes I've sent already: https://lore.kernel.org/linux-hardening/20241117044612.work.304-k...@kernel.org/ Having this for GCC 15 would be really lovely. *cross fingers* -Kees -- Kees Cook
Re: [PATCH] c: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
nated_string_initialization) Warning LangEnabledBy(C > ObjC,Wextra) > and adjust documentation, because say > -Wno-unterminated-string-initialization will no longer turn off that > warning, essentially -Wc++-compat and -Wunterminated-string-initialization > are then independent warnings with -Wc++-compat having priority over the > latter. > Or we could guard it with > warn_cxx_compat && warn_unterminated_string_initialization > but then the question is what to pass to second warning_at option, > because one needs both. > So, I think it is better to keep -Wc++-compat working as before (perhaps > with the extra descriptions of the two lengths) and then have this new > -Wunterminated-string-initialization warning implied by -Wextra which > does nothing in the rare case when -Wc++-compat is on, and otherwise > does the new stuff of warning except when initializing nonstring > objects. Okay, I will take a stab at getting this reorganized. Thanks for the review! -- Kees Cook
Re: 5th Ping: [Middle-end][PATCH v4 0/3][RFC]Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
On Mon, Jan 13, 2025 at 04:16:14PM +, Qing Zhao wrote: > Hi, > > This is the 5th ping of the middle end review of the patch set. > > Could you please take a look at the patch set for -fdiagnostics-details, and > provide > more advice on: > > A. Whether the middle-end design and implementation is reasonable and > extendable to more other > optimizations (including loop unrolling)? > B. If not, how to improve the current design to make it more extendable? > C. If yes, whether it’s okay to add this patches into GCC15, and then improve > it in GCC16? I'd really like to see this get landed. The Linux kernel has been using it quite successfully to find hard-to-understand corner cases where these warnings are being emitted. It's been really valuable. Is there anything I might be able to do to further help with testing? Getting it into GCC 15 would be amazing! Thanks, -Kees -- Kees Cook
[PATCH] c: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
When initializing a nonstring char array when compiled with
-Wunterminated-string-initialization the warning trips even when
truncating the trailing NUL character from the string constant. Only
warn about this when running under -Wc++-compat since under C++ we should
not initialize nonstrings from C strings.
PR c/117178
gcc/c/ChangeLog:
* c-typeck.cc (digest_init): Check for nonstring attribute and
avoid warning when only the trailing NUL is truncated.
(build_c_cast): Update function call prototype.
(store_init_value): Ditto.
(output_init_element): Ditto.
gcc/testsuite/ChangeLog:
* gcc.dg/Wunterminated-string-initialization.c: Update for
new warning text and check for nonstring cases.
* gcc.dg/Wunterminated-string-initialization-c++.c: Duplicate
C test for -Wc++-compat.
---
gcc/c/c-typeck.cc | 73 ---
.../Wunterminated-string-initialization-c++.c | 28 +++
.../Wunterminated-string-initialization.c | 26 ++-
3 files changed, 98 insertions(+), 29 deletions(-)
create mode 100644
gcc/testsuite/gcc.dg/Wunterminated-string-initialization-c++.c
diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index 10b02da8752d..5cd0c07b87b1 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -116,8 +116,8 @@ static void push_member_name (tree);
static int spelling_length (void);
static char *print_spelling (char *);
static void warning_init (location_t, int, const char *);
-static tree digest_init (location_t, tree, tree, tree, bool, bool, bool, bool,
-bool, bool);
+static tree digest_init (location_t, tree, tree, tree, tree, bool, bool, bool,
+bool, bool, bool);
static void output_init_element (location_t, tree, tree, bool, tree, tree,
bool,
bool, struct obstack *);
static void output_pending_init_elements (int, struct obstack *);
@@ -6731,7 +6731,7 @@ build_c_cast (location_t loc, tree type, tree expr)
t = build_constructor_single (type, field, t);
if (!maybe_const)
t = c_wrap_maybe_const (t, true);
- t = digest_init (loc, type, t,
+ t = digest_init (loc, field, type, t,
NULL_TREE, false, false, false, true, false, false);
TREE_CONSTANT (t) = TREE_CONSTANT (value);
return t;
@@ -8646,8 +8646,8 @@ store_init_value (location_t init_loc, tree decl, tree
init, tree origtype)
}
bool constexpr_p = (VAR_P (decl)
&& C_DECL_DECLARED_CONSTEXPR (decl));
- value = digest_init (init_loc, type, init, origtype, npc, int_const_expr,
- arith_const_expr, true,
+ value = digest_init (init_loc, decl, type, init, origtype, npc,
+ int_const_expr, arith_const_expr, true,
TREE_STATIC (decl) || constexpr_p, constexpr_p);
/* Store the expression if valid; else report error. */
@@ -8996,8 +8996,8 @@ check_constexpr_init (location_t loc, tree type, tree
init,
on initializers for 'constexpr' objects apply. */
static tree
-digest_init (location_t init_loc, tree type, tree init, tree origtype,
-bool null_pointer_constant, bool int_const_expr,
+digest_init (location_t init_loc, tree field, tree type, tree init,
+tree origtype, bool null_pointer_constant, bool int_const_expr,
bool arith_const_expr, bool strict_string,
bool require_constant, bool require_constexpr)
{
@@ -9132,27 +9132,46 @@ digest_init (location_t init_loc, tree type, tree init,
tree origtype,
&& TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST)
{
unsigned HOST_WIDE_INT len = TREE_STRING_LENGTH (inside_init);
- unsigned unit = TYPE_PRECISION (typ1) / BITS_PER_UNIT;
-
- /* Subtract the size of a single (possibly wide) character
-because it's ok to ignore the terminating null char
-that is counted in the length of the constant. */
- if (compare_tree_int (TYPE_SIZE_UNIT (type), len - unit) < 0)
- pedwarn_init (init_loc, 0,
- ("initializer-string for array of %qT "
- "is too long"), typ1);
- else if (warn_unterminated_string_initialization
- && compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
- warning_at (init_loc, OPT_Wunterminated_string_initialization,
- ("initializer-string for array of %qT "
-"is too long"), typ1);
+
if (compare_tree_int (TYPE_SIZE_UNIT (type), len) < 0)
{
- unsigned HOST_WIDE_INT size
- = tree_to_uhwi (TYPE_SIZE_UNIT (type));
- const char *p = TREE_STRING_POINTER (inside_init);
-
- inside_init = build_string (size, p
Re: [GCC16 stage 1][RFC][PATCH 0/3]extend "counted_by" attribute to pointer fields of structures
On Fri, Jan 17, 2025 at 01:27:41PM -0800, Bill Wendling wrote:
> On Fri, Jan 17, 2025 at 8:02 AM Qing Zhao wrote:
> > Joseph, Kees and Bill,
> >
> > I need your input on this.
> > > On Jan 17, 2025, at 10:12, Martin Uecker wrote:
> > >
> > > Am Donnerstag, dem 16.01.2025 um 21:18 + schrieb Qing Zhao:
> > >>
> > > ..
> > >>
> > >> Although in the previous discussion, I agreed with Martin that we should
> > >> use the
> > >> designator syntax (i.e, counted_by (.n) instead of counted_by (n)) for
> > >> the
> > >> counted_by attribute for pointer fields, after more consideration and
> > >> discussion
> > >> with Bill Wendling (who is working on the same work for CLANG), we
> > >> decided to
> > >> keep the current syntax of FAM for pointer fields. And leave the new
> > >> syntax (.n)
> > >> and more complicate expressions to a later work.
> > >>
> > > I think this would be a mistake. Once you have established the confusing
> > > syntax,
> > > it will not easily go away anymore. So I think you should use the
> > > unambiguous
> > > and future-prove syntax right away. Support for more complicated
> > > expressions
> > > could be left for later, of course.
> >
> > Personally I agree with you. -:)
> >
> > Actually we might need to use such syntax in the very beginning when adding
> > counted_by of FAM.
> > As I know, linux kernel community asked for the following new feature for
> > counted_by of FAM:
> >
> > struct fs_bulk {
> > ...
> > int len;
> > ...
> > }
> >
> > struct fc_bulk {
> > ...
> > struct fs_bulk fs_bulk;
> > struct fc fcs[] __counted_by(fs_bulk.len);
> > };
> >
> > i.e, the “counted_by” field is in the inner structure of the current
> > structure of the FAM.
> > With the current syntax, it’s not easy to extend to support this.
> >
> > But with the designator syntax, it might be much easier to be extended to
> > support this.
> >
> > So, Kees and Bill, what’s your opinion on this? I think that it’s better to
> > have a consistent interface between GCC
> > and Clang.
> >
> > Joseph, what’s your opinion on this new syntax? Shall we support the
> > designator syntax for counted_by attribute?
> >
> I've been thinking more about what we discussed regarding this and I
> have to agree with Martin that putting off using the new syntax for
> both FAMs and pointers will make things worse in the long run. We'll
> already have to deprecate the current syntax in compilers for code
> using the current syntax, but it may not be too late. Linux is pretty
> much the main user of this feature, and we have pretty good control
> over how it's used there.
While it'll take a bit longer, I am convinced as well. It will let us do
things we keep tripping over (like fs_bulk.len example above).
> The one wrinkle is that Apple allows for full expressions, not just a
> field designator---and they're currently using full expressions.
> AFAIK, that code is internal to Apple, but they want to externalize
> it. So we'll eventually have to come up with some sort of plan for how
> to deal with it probably sooner rather than later. I'll open up a
> discussion with them about it.
Gaining access to global variables is another gap Linux has -- e.g. we
have arrays that are sized by the global number-of-cpus variable. :)
--
Kees Cook
Re: [PATCH] c, v2: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
On Fri, Mar 07, 2025 at 09:41:15PM -0800, Kees Cook wrote:
> On Tue, Feb 18, 2025 at 11:51:41AM -0800, Kees Cook wrote:
> > On Fri, Feb 14, 2025 at 11:21:07AM +0100, Jakub Jelinek wrote:
> > > On Thu, Feb 13, 2025 at 02:10:25PM +0100, Jakub Jelinek wrote:
> > > > Kees, are you submitting this under assignment to FSF (maybe the Google
> > > > one
> > > > if it has one) or DCO? See https://gcc.gnu.org/contribute.html#legal
> > > > for details. If DCO, can you add your Signed-off-by: tag for it?
> > > >
> > > > So far lightly tested, ok for trunk if it passes bootstrap/regtest?
> > >
> > > Bootstrapped/regtested on x86_64-linux and i686-linux successfully.
> >
> > Thank you for getting this done! I really appreciate having this
> > available. I'll give it a spin. :)
>
> Sorry this took me so long. I finally tested this and while it does
> allow setting nonstring on multi-dim char arrays, it looks like the
> initializer check is still broken:
>
> $ cat multi.c
> const char table_sigs[][4] __attribute__((nonstring)) = {
> "BERT", "BGRT", "CPEP", "ECDT"
> };
> $ gcc -o multi.o -c multi.c -O2 -Wall -Wunterminated-string-initialization
> multi.c:2:9: warning: initializer-string for array of 'char' truncates NUL
> terminator but destination lacks 'nonstring' attribute (5 chars into 4
> available) [-Wunterminated-string-initialization]
> 2 | "BERT", "BGRT", "CPEP", "ECDT"
> | ^~
> multi.c:2:17: warning: initializer-string for array of 'char' truncates NUL
> terminator but destination lacks 'nonstring' attribute (5 chars into 4
> available) [-Wunterminated-string-initialization]
> 2 | "BERT", "BGRT", "CPEP", "ECDT"
> | ^~
> ...
>
> It looks like get_attr_nonstring_decl() still isn't correctly finding
> the attribute. (It looks like multi-dimensional static initializers
> tests that drop the NUL are missing in commit ab714e60870d.)
>
> And to answer your earlier question about Linux's use, while we have
> the __nonstring macro for applying the attribute, I intend to add
> __nonstring_array that is version-checked against GCC 15.
Hi! Thanks again for continuing to poke at this. Even with commit
1301e18f69ce ("gimple-ssa-warn-access: Adjust maybe_warn_nonstring_arg
for nonstring multidimensional arrays [PR117178]"), this is still not
working. Does the above test pass for you? I still get warnings with
the latest ToT GCC.
Can you add a test like the above? Perhaps this, which should emit no
warning:
diff --git a/gcc/testsuite/c-c++-common/attr-nonstring-11.c
b/gcc/testsuite/c-c++-common/attr-nonstring-11.c
index 745e99382fe3..5e722df0d641 100644
--- a/gcc/testsuite/c-c++-common/attr-nonstring-11.c
+++ b/gcc/testsuite/c-c++-common/attr-nonstring-11.c
@@ -417,6 +417,7 @@ void test_strlen (struct MemArrays *p, char *s NONSTRING,
size_t n)
}
}
+const char drop_nul[][4] __attribute__ ((nonstring)) = { "ABCD", "1234" };
void test_strnlen (struct MemArrays *p, size_t n)
{
--
Kees Cook
Re: [PATCH] sanitizer: Store no_sanitize attribute value in uint32 instead of unsigned
On April 22, 2025 12:08:51 AM PDT, Sam James wrote:
>Kees Cook writes:
>
>> On Thu, Apr 10, 2025 at 05:17:51PM -0700, Keith Packard wrote:
>>> A target using 16-bit ints won't have enough bits to hold the whole
>>> flag_sanitize set. Be explicit about using uint32 for the attribute data.
>>>
>>> Signed-off-by: Keith Packard
>>> ---
>>> gcc/c-family/c-attribs.cc | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
>>> index 5a0e3d328ba..2a4ae10838a 100644
>>> --- a/gcc/c-family/c-attribs.cc
>>> +++ b/gcc/c-family/c-attribs.cc
>>> @@ -1420,12 +1420,12 @@ add_no_sanitize_value (tree node, unsigned int
>>> flags)
>>>if (flags == old_value)
>>> return;
>>>
>>> - TREE_VALUE (attr) = build_int_cst (unsigned_type_node, flags);
>>> + TREE_VALUE (attr) = build_int_cst (uint32_type_node, flags);
>>> }
>>>else
>>> DECL_ATTRIBUTES (node)
>>>= tree_cons (get_identifier ("no_sanitize"),
>>> - build_int_cst (unsigned_type_node, flags),
>>> + build_int_cst (uint32_type_node, flags),
>>>DECL_ATTRIBUTES (node));
>>> }
>>
>> This looks correct to me. Martin, is this something you (or someone
>> else) can get committed for GCC 15?
>
>Martin isn't involved with GCC development anymore. But it's not clear
Ah! Okay, it seemed like he had some relatively recent commits.
>to me why this is critical for GCC 15. (Any further patches for 15.1
>need to be both approved and then approved by release managers).
It's not critical. I was thinking it would be nice to have in 15 as it was a
small/easy change.
>Did you need this for something on the Linux side?
No, this is not needed for Linux. I have been following Keith's use of the
sanitizers on the picolibc project[1] and thought I might be able to help find
someone that could commit this fix.
>More likely that if approved, it could then go into 15.2 which will be
>in a few months (not a year unlike > X.3).
Any release is fine. My suggestion for 15 was just based on seeing it as a
trivial fix and not sufficiently understanding the GCC release process. :)
Thanks for looking at it!
-Kees
[1] https://keithp.com/blogs/sanitizer-fun/
--
Kees Cook
Re: [GCC16 stage 1][PATCH v2 0/3] extend "counted_by" attribute to pointer fields of structures
On Thu, Apr 24, 2025 at 04:36:14PM +, Qing Zhao wrote:
>
> > On Apr 24, 2025, at 11:59, Martin Uecker wrote:
> >
> > Am Donnerstag, dem 24.04.2025 um 15:15 + schrieb Qing Zhao:
> >> Hi,
> >>
> >> Kees reported a segmentation failure when he used the patch to compiler
> >> kernel,
> >> and the reduced the testing case is something like the following:
> >>
> >> struct f {
> >> void *g __attribute__((__counted_by__(h)));
> >> long h;
> >> };
> >>
> >> extern struct f *my_alloc (int);
> >>
> >> int i(void) {
> >> struct f *iov = my_alloc (10);
> >> int *j = (int *)iov->g;
> >> return __builtin_dynamic_object_size(iov->g, 0);
> >> }
> >>
> >> Basically, the problem is relating to the pointee type of the pointer
> >> array being “void”,
> >> As a result, the element size of the array is not available in the IR.
> >> Therefore segmentation
> >> fault when calculating the size of the whole object.
> >>
> >> Although it’s easy to fix this segmentation failure, I am not quite sure
> >> what’s the best
> >> solution to this issue:
> >>
> >> 1. Reject such usage of “counted_by” in the very beginning by reporting
> >> warning to the
> >> User, and delete the counted_by attribute from the field.
> >>
> >> Or:
> >>
> >> 2. Accept such usage, but issue warnings when calculating the object_size
> >> in Middle-end.
> >>
> >> Personally, I prefer the above 1 since I think that when the pointee type
> >> is void, we don’t know
> >> The type of the element of the pointer array, there is no way to decide
> >> the size of the pointer array.
> >>
> >> So, the counted_by information is not useful for the
> >> __builtin_dynamic_object_size.
> >>
> >> But I am not sure whether the counted_by still can be used for bound
> >> sanitizer?
> >>
> >> Thanks for suggestions and help.
> >
> > GNU C allows pointer arithmetic and sizeof on void pointers and
> > that treats void as having size 1. So you could also allow counted_by
> > and assume as size 1 for void.
> >
> > https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
>
> Okay, thanks for the info.
> So,
> 1. should we issue warnings when doing this?
Please don't, Linux would very much like to track these allocation sizes
still. Performing pointer arithmetic and bounds checking (via __bdos) on
"void *" is wanted (and such a calculation was what tripped the
segfault).
> 2. If the compilation option is explicitly asking for standard C,
> shall we issue warning and delete the counted_by attribute from the field?
I think it needs to stay attached for __bdos. And from the looks of it,
even array access works with 1-byte values too:
extern void *ptr;
void *foo(int num) {
return &ptr[num];
}
The assembly output of this shows it's doing byte addition. Clang
doesn't warn about this, but GCC does:
:5:16: warning: dereferencing 'void *' pointer
5 | return &ptr[num];
|^
So, I think even the bounds sanitizer should handle it, even if a
warning ultimately gets emitted.
-Kees
--
Kees Cook
Re: [Stage 1][Middle-end][PATCH v5 0/3] Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
On Mon, Apr 07, 2025 at 03:04:56PM +, Qing Zhao wrote: > Adding -fdiagnotics-details into GCC to provide more hints to the > end users on how the warnings come from, in order to help the user > to locate the exact location in source code on the specific warnings > due to compiler optimizations. FWIW, I've been very happily using this to find and squash tricky bugs in Linux for a few months now. This is finally getting us close to being able to enable -Warray-bounds globally. :) https://lore.kernel.org/all/?q=%22-fdiagnostic%22+%22-details%22 -- Kees Cook
Re: [GCC16 stage 1][PATCH v2 0/3] extend "counted_by" attribute to pointer fields of structures
On Thu, Apr 24, 2025 at 06:06:03PM +, Qing Zhao wrote:
>
>
> > On Apr 24, 2025, at 13:07, Kees Cook wrote:
> >
> > On Thu, Apr 24, 2025 at 04:36:14PM +, Qing Zhao wrote:
> >>
> >>> On Apr 24, 2025, at 11:59, Martin Uecker wrote:
> >>>
> >>> Am Donnerstag, dem 24.04.2025 um 15:15 + schrieb Qing Zhao:
> >>>> Hi,
> >>>>
> >>>> Kees reported a segmentation failure when he used the patch to compiler
> >>>> kernel,
> >>>> and the reduced the testing case is something like the following:
> >>>>
> >>>> struct f {
> >>>> void *g __attribute__((__counted_by__(h)));
> >>>> long h;
> >>>> };
> >>>>
> >>>> extern struct f *my_alloc (int);
> >>>>
> >>>> int i(void) {
> >>>> struct f *iov = my_alloc (10);
> >>>> int *j = (int *)iov->g;
> >>>> return __builtin_dynamic_object_size(iov->g, 0);
> >>>> }
> >>>>
> >>>> Basically, the problem is relating to the pointee type of the pointer
> >>>> array being “void”,
> >>>> As a result, the element size of the array is not available in the IR.
> >>>> Therefore segmentation
> >>>> fault when calculating the size of the whole object.
> >>>>
> >>>> Although it’s easy to fix this segmentation failure, I am not quite sure
> >>>> what’s the best
> >>>> solution to this issue:
> >>>>
> >>>> 1. Reject such usage of “counted_by” in the very beginning by reporting
> >>>> warning to the
> >>>> User, and delete the counted_by attribute from the field.
> >>>>
> >>>> Or:
> >>>>
> >>>> 2. Accept such usage, but issue warnings when calculating the
> >>>> object_size in Middle-end.
> >>>>
> >>>> Personally, I prefer the above 1 since I think that when the pointee
> >>>> type is void, we don’t know
> >>>> The type of the element of the pointer array, there is no way to decide
> >>>> the size of the pointer array.
> >>>>
> >>>> So, the counted_by information is not useful for the
> >>>> __builtin_dynamic_object_size.
> >>>>
> >>>> But I am not sure whether the counted_by still can be used for bound
> >>>> sanitizer?
> >>>>
> >>>> Thanks for suggestions and help.
> >>>
> >>> GNU C allows pointer arithmetic and sizeof on void pointers and
> >>> that treats void as having size 1. So you could also allow counted_by
> >>> and assume as size 1 for void.
> >>>
> >>> https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> >>
> >> Okay, thanks for the info.
> >> So,
> >> 1. should we issue warnings when doing this?
> >
> > Please don't, Linux would very much like to track these allocation sizes
> > still. Performing pointer arithmetic and bounds checking (via __bdos) on
> > "void *" is wanted (and such a calculation was what tripped the
> > segfault).
>
> My previous question was: -:)
>
> When we accept the “void” pointee type and provide
> __builtin_dynamic_object_size for
> such pointers (treating it as 1 byte) shall we issue a warning to users to
> warn them that the void pointee type is
> Accepted and treated as size 1?
>
> Or just silently handle such case as normal?
I think it should be silently handled. Other such "void is 1 byte" cases
don't warn.
> >> 2. If the compilation option is explicitly asking for standard C,
> >>shall we issue warning and delete the counted_by attribute from the
> >> field?
> >
> > I think it needs to stay attached for __bdos. And from the looks of it,
> > even array access works with 1-byte values too:
> >
> > extern void *ptr;
> > void *foo(int num) {
> >return &ptr[num];
> > }
> >
> > The assembly output of this shows it's doing byte addition. Clang
> > doesn't warn about this, but GCC does:
> >
> > :5:16: warning: dereferencing 'void *' pointer
> >5 | return &ptr[num];
> > |^
> >
> > So, I think even the bounds sanitizer should handle it, even if a
> > warning ultimately gets emitted.
>
> Okay. I will also handle the void in bounds sanitizer by treating element
> size as 1 byte.
Great, that should work well for Linux, and is, I think, the least
surprising result. :)
>
> My previous question was:
>
> Since this is only a GNU extension, I am wondering under the situation that
> No GNU extension
> Is allowed, shall we issue warnings and delete the counted_by attribute?
Oh, like, generally? What GCC option disables GNU extensions?
-Kees
--
Kees Cook
Re: [GCC16 stage 1][PATCH v2 0/3] extend "counted_by" attribute to pointer fields of structures
On April 24, 2025 1:44:23 PM PDT, Qing Zhao wrote:
>
>
>> On Apr 24, 2025, at 15:43, Bill Wendling wrote:
>>
>> On Thu, Apr 24, 2025 at 8:15 AM Qing Zhao wrote:
>>>
>>> Hi,
>>>
>>> Kees reported a segmentation failure when he used the patch to compiler
>>> kernel,
>>> and the reduced the testing case is something like the following:
>>>
>>> struct f {
>>> void *g __attribute__((__counted_by__(h)));
>>> long h;
>>> };
>>>
>>> extern struct f *my_alloc (int);
>>>
>>> int i(void) {
>>> struct f *iov = my_alloc (10);
>>> int *j = (int *)iov->g;
>>> return __builtin_dynamic_object_size(iov->g, 0);
>>> }
>>>
>>> Basically, the problem is relating to the pointee type of the pointer array
>>> being “void”,
>>> As a result, the element size of the array is not available in the IR.
>>> Therefore segmentation
>>> fault when calculating the size of the whole object.
>>>
>>> Although it’s easy to fix this segmentation failure, I am not quite sure
>>> what’s the best
>>> solution to this issue:
>>>
>>> 1. Reject such usage of “counted_by” in the very beginning by reporting
>>> warning to the
>>> User, and delete the counted_by attribute from the field.
>>>
>>> Or:
>>>
>>> 2. Accept such usage, but issue warnings when calculating the object_size
>>> in Middle-end.
>>>
>>> Personally, I prefer the above 1 since I think that when the pointee type
>>> is void, we don’t know
>>> The type of the element of the pointer array, there is no way to decide the
>>> size of the pointer array.
>>>
>>> So, the counted_by information is not useful for the
>>> __builtin_dynamic_object_size.
>>>
>>> But I am not sure whether the counted_by still can be used for bound
>>> sanitizer?
>>>
>>> Thanks for suggestions and help.
>>>
>> Clang supports __sized_by that can handle a 'void *', where it defaults to
>> 'u8'.
I would like to be able to use counted_by (and not sized_by) so that users of
the annotation don't need to have to change the marking just because it's "void
*". Everything else operates on "void *" as if it were u8 ...
Regardless, ignoring "void *", the rest of my initial testing (of both GCC and
Clang) is positive. The test cases are all behaving as expected! Yay! :) I will
try to construct some more goofy stuff to find more corner cases.
And at some future point we may want to think about -fsanitize=pointer-overflow
using this information too, to catch arithmetic and increments past the
bounds...
struct foo {
u8 *buf __counted_by(len);
int len;
} str;
u8 *walk;
str->buf = malloc(10);
str->len = 10;
walk = str->buf + 12; // trip!
for (walk = str->buf; ; walk++) // trip after 10 loops
;
-Kees
--
Kees Cook
Re: [GCC16 stage 1][PATCH v2 0/3] extend "counted_by" attribute to pointer fields of structures
On Fri, Apr 25, 2025 at 04:56:52PM +, Qing Zhao wrote:
>
>
> > On Apr 24, 2025, at 13:07, Kees Cook wrote:
> >
> > On Thu, Apr 24, 2025 at 04:36:14PM +, Qing Zhao wrote:
> >>
> >>> On Apr 24, 2025, at 11:59, Martin Uecker wrote:
> >>>
> >>> Am Donnerstag, dem 24.04.2025 um 15:15 + schrieb Qing Zhao:
> >>>> Hi,
> >>>>
> >>>> Kees reported a segmentation failure when he used the patch to compiler
> >>>> kernel,
> >>>> and the reduced the testing case is something like the following:
> >>>>
> >>>> struct f {
> >>>> void *g __attribute__((__counted_by__(h)));
> >>>> long h;
> >>>> };
> >>>>
> >>>> extern struct f *my_alloc (int);
> >>>>
> >>>> int i(void) {
> >>>> struct f *iov = my_alloc (10);
> >>>> int *j = (int *)iov->g;
> >>>> return __builtin_dynamic_object_size(iov->g, 0);
> >>>> }
> >>>>
> >>>> Basically, the problem is relating to the pointee type of the pointer
> >>>> array being “void”,
> >>>> As a result, the element size of the array is not available in the IR.
> >>>> Therefore segmentation
> >>>> fault when calculating the size of the whole object.
> >>>>
> >>>> Although it’s easy to fix this segmentation failure, I am not quite sure
> >>>> what’s the best
> >>>> solution to this issue:
> >>>>
> >>>> 1. Reject such usage of “counted_by” in the very beginning by reporting
> >>>> warning to the
> >>>> User, and delete the counted_by attribute from the field.
> >>>>
> >>>> Or:
> >>>>
> >>>> 2. Accept such usage, but issue warnings when calculating the
> >>>> object_size in Middle-end.
> >>>>
> >>>> Personally, I prefer the above 1 since I think that when the pointee
> >>>> type is void, we don’t know
> >>>> The type of the element of the pointer array, there is no way to decide
> >>>> the size of the pointer array.
> >>>>
> >>>> So, the counted_by information is not useful for the
> >>>> __builtin_dynamic_object_size.
> >>>>
> >>>> But I am not sure whether the counted_by still can be used for bound
> >>>> sanitizer?
> >>>>
> >>>> Thanks for suggestions and help.
> >>>
> >>> GNU C allows pointer arithmetic and sizeof on void pointers and
> >>> that treats void as having size 1. So you could also allow counted_by
> >>> and assume as size 1 for void.
> >>>
> >>> https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> >>
> >> Okay, thanks for the info.
> >> So,
> >> 1. should we issue warnings when doing this?
> >
> > Please don't, Linux would very much like to track these allocation sizes
> > still. Performing pointer arithmetic and bounds checking (via __bdos) on
> > "void *" is wanted (and such a calculation was what tripped the
> > segfault).
> >
> >> 2. If the compilation option is explicitly asking for standard C,
> >>shall we issue warning and delete the counted_by attribute from the
> >> field?
> >
> > I think it needs to stay attached for __bdos. And from the looks of it,
> > even array access works with 1-byte values too:
> >
> > extern void *ptr;
> > void *foo(int num) {
> >return &ptr[num];
> > }
> >
> > The assembly output of this shows it's doing byte addition. Clang
> > doesn't warn about this, but GCC does:
> >
> > :5:16: warning: dereferencing 'void *' pointer
> >5 | return &ptr[num];
> > |^
> >
> > So, I think even the bounds sanitizer should handle it, even if a
> > warning ultimately gets emitted.
>
> I tried to come up with a testing case for array sanitizer on void pointers
> as following:
>
> #include
>
> struct annotated {
> int b;
> void *c __attribute__ ((counted_by (b)));
> } *array_annotated;
>
> void __attribute__((__noinline__)) setup (int annotated_count)
> {
> array_annotated
> = (struct annotated *)malloc (sizeof (struct annotated));
> array_annotated->c = malloc (sizeof (char) * annotated_count);
> array_annotated->b = annotated_count;
>
> return;
> }
>
> void __attribute__((__noinline__)) test (int annotated_index)
> {
> array_annotated->c[annotated_index] = 2;
Have this be:
void * __attribute__((__noinline__)) test (int annotated_index)
{
return &array_annotated->c[annotated_index];
}
We stay dereferenced, but we can do take the address.
Actually, the index will likely need to be "annotated_index + 1" because
of this bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=119132
(I still don't accept this as "invalid", but I have other hills to die on)
--
Kees Cook
Re: [RFC] [C]New syntax for the argument of counted_by attribute for C language
On Tue, Apr 15, 2025 at 09:07:44PM +0200, Martin Uecker wrote:
> Am Dienstag, dem 15.04.2025 um 14:50 +0200 schrieb Michael Matz:
> > Hello,
> ...
>
> > > struct A {
> > > int *buf __counted_by(len); // 'len' *must* be in the struct.
> > > int len;
> > > };
> >
> > ... means that we would have to implement general delayed parsing for
> > expressions in C parsers.
>
> I have to agree with Michael. This was the main reason
> we rejected the original approach.
>
> I also think consistency with general syntax for arrays in structs
> is far more important for C than consistency for the special case of
> having only one identifier in counted_by.
Okay, so I think the generally recognized way forward is with two
attributes:
counted_by(struct_member)
and
counted_by_expr(type struct_member; ...; expression)
This leaves flexible array members with counted_by unchanged from
current behavior.
Questions I am left with:
1) When applying counted_by to pointer members, are out-of-order member
declarations expected to be handled? As in, is this expected to be valid?
struct foo {
struct bar *p __attribute__((counted_by(count)));
int count;
};
1.A) If it is _not_ valid, is it valid to use it when the member has
been declared earlier? Such as:
struct foo {
int count;
struct bar *p __attribute__((counted_by(count)));
};
1.B) If "1" isn't valid, but "1.A" is valid, I would expect that way to
allow the member ordering in "1" is through counted_by_expr? For example:
struct foo {
struct bar *p __attribute__((counted_by_expr(int count;
count)));
int count;
};
1.C) If "1" isn't valid, and "1.A" isn't valid, then counted_by of
pointer members must always use counted_by_expr. Is that expected?
(I ask because it seems like a potentially weird case there member order
forces choosing between two differently named attributes. It'd be really
nice if "1" could be valid.)
2) For all counted_by of pointer members, I understand this to only be
about the parsing step, not further analysis where the full sizes of
all objects will need to be known. Which means that this is valid:
struct bar; // empty declaration
struct foo {
struct bar *p __attribute__((counted_by_expr(int count;
count)));
int count;
};
...
// defined after being referenced by counted_by_expr above
struct bar {
int a, b, c;
struct foo *p;
};
Is that correct?
3) It seems it will be possible to provide a "singleton" alias to
indicate that a given pointer member is not an array of objects, but
rather a pointer to a single object instance:
struct bar {
int a, b, c;
struct foo *p __attribute__((counted_by_expr(1)));
};
Is that correct? (This will be useful once we can apply counted_by to
function arguments...)
4) If there are type mismatches between the counted_by_expr struct
member declaration and the later actual struct member declaration, I
assume that will be a hard error. For example, this would fail to compile:
struct foo {
struct bar *p __attribute__((counted_by_expr(int count;
count)));
unsigned long count;
};
Is that correct? It feels like if we're already able to do this analysis,
then "1" should be possible also. Perhaps I'm misunderstanding something
about the parser.
Thanks!
-Kees
--
Kees Cook
Re: [RFC] [C]New syntax for the argument of counted_by attribute for C language
On Tue, Apr 15, 2025 at 09:05:20PM +, Qing Zhao wrote:
> > On Apr 15, 2025, at 16:35, Kees Cook wrote:
> > 1) When applying counted_by to pointer members, are out-of-order member
> > declarations expected to be handled? As in, is this expected to be valid?
> >
> > struct foo {
> > struct bar *p __attribute__((counted_by(count)));
> > int count;
> > };
>
> Yes, this is valid.
> Given a lone identifier, you will use “counted_by” attribute the same as the
> counted_by for FAM, and this identifier will be looked up inside the
> enclosing
> structure. (Even when the counted_by field is AFTER the pointer).
>
> The patch I sent a while ago on the counted_by for pointer filed of a
> structure:
> https://gcc.gnu.org/pipermail/gcc-patches/2025-January/673837.html
>
> Have supported this without issue.
Hurray! Okay, I think that will make this quite friendly to use.
> > 4) If there are type mismatches between the counted_by_expr struct
> > member declaration and the later actual struct member declaration, I
> > assume that will be a hard error. For example, this would fail to compile:
> >
> > struct foo {
> > struct bar *p __attribute__((counted_by_expr(int count; count)));
> > unsigned long count;
> > };
> >
> > Is that correct?
>
> I guess that the hard error will be issued when the parser sees “unsigned
> long count” which
> Is conflict with the previous seen “int count” inside counted_by_expr.
>
> But I am not sure on this.
I guess the mismatch could be ignored/warned as long as the actual
struct member type is used in the end.
Thanks!
--
Kees Cook
Re: [PATCH] sanitizer: Store no_sanitize attribute value in uint32 instead of unsigned
On Thu, Apr 10, 2025 at 05:17:51PM -0700, Keith Packard wrote:
> A target using 16-bit ints won't have enough bits to hold the whole
> flag_sanitize set. Be explicit about using uint32 for the attribute data.
>
> Signed-off-by: Keith Packard
> ---
> gcc/c-family/c-attribs.cc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc
> index 5a0e3d328ba..2a4ae10838a 100644
> --- a/gcc/c-family/c-attribs.cc
> +++ b/gcc/c-family/c-attribs.cc
> @@ -1420,12 +1420,12 @@ add_no_sanitize_value (tree node, unsigned int flags)
>if (flags == old_value)
> return;
>
> - TREE_VALUE (attr) = build_int_cst (unsigned_type_node, flags);
> + TREE_VALUE (attr) = build_int_cst (uint32_type_node, flags);
> }
>else
> DECL_ATTRIBUTES (node)
>= tree_cons (get_identifier ("no_sanitize"),
> -build_int_cst (unsigned_type_node, flags),
> +build_int_cst (uint32_type_node, flags),
> DECL_ATTRIBUTES (node));
> }
This looks correct to me. Martin, is this something you (or someone
else) can get committed for GCC 15?
Thanks!
-Kees
P.S. Sorry for the double email Martin, it took me a while to find mbox;
I'm not subscribed to gcc-patches. Thankfully the patchwork has it:
https://patchwork.sourceware.org/project/gcc/patch/20250411001751.141494-1-kei...@keithp.com/
--
Kees Cook
Re: [PATCH] c, v2: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
On Fri, Feb 14, 2025 at 11:21:07AM +0100, Jakub Jelinek wrote: > On Thu, Feb 13, 2025 at 02:10:25PM +0100, Jakub Jelinek wrote: > > Kees, are you submitting this under assignment to FSF (maybe the Google one > > if it has one) or DCO? See https://gcc.gnu.org/contribute.html#legal > > for details. If DCO, can you add your Signed-off-by: tag for it? > > > > So far lightly tested, ok for trunk if it passes bootstrap/regtest? > > Bootstrapped/regtested on x86_64-linux and i686-linux successfully. Thank you for getting this done! I really appreciate having this available. I'll give it a spin. :) -- Kees Cook
Re: [PATCH] c, v2: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
On Tue, Feb 18, 2025 at 11:51:41AM -0800, Kees Cook wrote:
> On Fri, Feb 14, 2025 at 11:21:07AM +0100, Jakub Jelinek wrote:
> > On Thu, Feb 13, 2025 at 02:10:25PM +0100, Jakub Jelinek wrote:
> > > Kees, are you submitting this under assignment to FSF (maybe the Google
> > > one
> > > if it has one) or DCO? See https://gcc.gnu.org/contribute.html#legal
> > > for details. If DCO, can you add your Signed-off-by: tag for it?
> > >
> > > So far lightly tested, ok for trunk if it passes bootstrap/regtest?
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux successfully.
>
> Thank you for getting this done! I really appreciate having this
> available. I'll give it a spin. :)
Sorry this took me so long. I finally tested this and while it does
allow setting nonstring on multi-dim char arrays, it looks like the
initializer check is still broken:
$ cat multi.c
const char table_sigs[][4] __attribute__((nonstring)) = {
"BERT", "BGRT", "CPEP", "ECDT"
};
$ gcc -o multi.o -c multi.c -O2 -Wall -Wunterminated-string-initialization
multi.c:2:9: warning: initializer-string for array of 'char' truncates NUL
terminator but destination lacks 'nonstring' attribute (5 chars into 4
available) [-Wunterminated-string-initialization]
2 | "BERT", "BGRT", "CPEP", "ECDT"
| ^~
multi.c:2:17: warning: initializer-string for array of 'char' truncates NUL
terminator but destination lacks 'nonstring' attribute (5 chars into 4
available) [-Wunterminated-string-initialization]
2 | "BERT", "BGRT", "CPEP", "ECDT"
| ^~
...
It looks like get_attr_nonstring_decl() still isn't correctly finding
the attribute. (It looks like multi-dimensional static initializers
tests that drop the NUL are missing in commit ab714e60870d.)
And to answer your earlier question about Linux's use, while we have
the __nonstring macro for applying the attribute, I intend to add
__nonstring_array that is version-checked against GCC 15.
-Kees
--
Kees Cook
Re: [PATCH] c, v2: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
On February 14, 2025 2:21:07 AM PST, Jakub Jelinek wrote: >On Thu, Feb 13, 2025 at 02:10:25PM +0100, Jakub Jelinek wrote: >> Kees, are you submitting this under assignment to FSF (maybe the Google one >> if it has one) or DCO? See https://gcc.gnu.org/contribute.html#legal >> for details. If DCO, can you add your Signed-off-by: tag for it? >> >> So far lightly tested, ok for trunk if it passes bootstrap/regtest? > >Bootstrapped/regtested on x86_64-linux and i686-linux successfully. > >> 2025-02-13 Kees Cook >> Jakub Jelinek >> >> PR c/117178 >> gcc/ >> * doc/invoke.texi (Wunterminated-string-initialization): Document >> the new interaction between this warning and -Wc++-compat and that >> initialization of decls with nonstring attribute aren't warned about. >> gcc/c-family/ >> * c.opt (Wunterminated-string-initialization): Don't depend on >> -Wc++-compat. >> gcc/c/ >> * c-typeck.cc (digest_init): Add DECL argument. Adjust wording of >> pedwarn_init for too long strings and provide details on the lengths, >> for string literals where just the trailing NULL doesn't fit warn for >> warn_cxx_compat with OPT_Wc___compat, wording which mentions "for C++" >> and provides details on lengths, otherwise for >> warn_unterminated_string_initialization adjust the warning, provide >> details on lengths and don't warn if get_attr_nonstring_decl (decl). >> (build_c_cast, store_init_value, output_init_element): Adjust >> digest_init callers. >> gcc/testsuite/ >> * gcc.dg/Wunterminated-string-initialization.c: Add additional test >> coverage. >> * gcc.dg/Wcxx-compat-14.c: Check in dg-warning for "for C++" part of >> the diagnostics. >> * gcc.dg/Wcxx-compat-23.c: New test. >> * gcc.dg/Wcxx-compat-24.c: New test. > > Jakub Jakub reminded me that I need to fulfill the DCO requirements for the earlier version of this patch. So, for that: Signed-off-by: Kees Cook Thanks! -Kees -- Kees Cook
Re: [RFC][C]New syntax for the argument of counted_by attribute for C language
tten to add “__self.”
I don't have a solution for this risk, so from that perspective, it does
appear that member names used in counted_by must not alias global names.
But I think that is a new scope, with very weird semantics. It means
that if a global appears in any header that happens to match the member
name used in counted_by, the compilation fails. This seems risky too.
But I think that such a construction results in a completely new scope
(a namespace where all globals are merged into a given struct's member
namespace) and creates a very fragile state. With __self, we are
exclusively using global scope with a special "global" named __self.
(It cannot be local scope because then the meaning of the counted_by
expression would be expected to change if there were any local variables
aliasing global variables, which is even more wild.)
> because it’s so
> intuitive to use the member name inside the attributes (I know what’s
> “intuitive" depends on people’s background, but that’s what we
> observed from massive adoption experience within Apple). This leaves
> the feature error-prone, because the most intuitive syntax for bounds
> annotations will be compiled into a different meaning (using the global
> as the size instead of the peer member). So we should really diagnose
> it even if we add “__self" to avoid the mistake.
So, I think your earlier observation is correct about the "common" case
for counted_by: it is just a single member name and nothing else: no
expression, no globals, etc etc. I think the answer here is exactly what
I suggested above, making it a macro issue, and leaving the feature
itself to require __self. Then authors can easily deal with the common
case not needing __self, but complex expressions can be constructed
without them being ambiguous:
#define __counted_by(x) __attribute__((__counted_by__(__self.(x
#define __counted_by_expr(x) __attribute__((__counted_by__(x)))
struct easy {
int length;
u8 bytes[] __counted_by(length);
};
struct complex {
u64 long packet_info;
u32 flags;
u16 whole_size;
u8 bytes[] __counted_by_expr(__self.whole_size - sizeof(struct complex));
};
u32 nr_cpus;
struct sys_info {
unsigned long details;
struct cpu_info cpu[] __counted_by_expr(nr_cpus);
};
> Now, if we always diagnose it, then the lookup order doesn’t really
> matter anymore. That means we will have an option to keep the current
> lookup rule of C, and pick up the member name only when the global name
> is not available (just one possible option). I see “__self.” being
> used as a suppression mechanism if the programmer cannot change the name
> of the conflicting global or member. But that doesn’t mean “__self”
> should be a default way of writing the code. Suppression mechanisms are
> typically only used to suppress the warnings and disambiguate. And this
> would mean we also need a way to disambiguate it to mean global. C++
> already has `::` but C doesn’t currently have a scope qualifier but
> in order to use this new bounds safety feature, we may need to invent
> something. Adding a new syntax is a risk so until we standardize it I
> would suggest something like `__builtin_global_ref()`
I'm not opposed to adding something like __builtin_global_ref(), but I
don't think it really solves the confusion related to the existing
scope resolution, as Qing showed with a VLA that uses local scope.
void boo (int k)
{
const int n = 10; // a local variable n
struct foo {
int n; // a member variable n
int a[n + 10]; // for VLA, this n refers to the local variable n.
char b[] __counted_by(n); // aaagh
};
...
}
However, my "hide __self in the macro" solution doesn't make the above
any less weird to read, so perhaps this isn't as useful an argument as
I'd like. :)
> [...]
> We have a way to not change the meaning of the existing code without
> introducing a new syntax, but diagnosing already error-prone code that
> should apply to both VLAs and bounds annotations. We are planning to
> write up a proposal to the C standard soon.
What does "soon" mean? Can you send it out next week?
It sounds like you're proposing adding "instance scope", which merges an
object's member names with the global scope variable names. And to access
global scope in the face of an aliased name, __builtin_global_ref(NAME)
would be needed. (Such a builtin would be useful in other areas too,
e.g. local scope to access an aliased name...)
It does seem like this solves all the same problems, but I still don't
like how fragile it is in the face of adding a global that might alias a
used member name. When this happens in local scope, it's limited to the
given function, and change any behaviors. For instance scope it means
that suddenly no globals can be added that alias with used member names.
And on the GCC side, all "instance scope" usage would just have parsing
delayed until the end of the struct parisng.
Thanks for the discussion!
-Kees
[1] https://clang.llvm.org/docs/BoundsSafety.html
--
Kees Cook
Re: [PATCH] c, v2: do not warn about truncating NUL char when initializing nonstring arrays [PR117178]
On Mon, Mar 10, 2025 at 01:03:44PM -0700, Kees Cook wrote:
> Hi! Thanks again for continuing to poke at this. Even with commit
> 1301e18f69ce ("gimple-ssa-warn-access: Adjust maybe_warn_nonstring_arg
> for nonstring multidimensional arrays [PR117178]"), this is still not
> working. Does the above test pass for you? I still get warnings with
> the latest ToT GCC.
Oh! I see from the bug that that commit wasn't intended to fix it. I see
the proposed patch[1] there. I will go test this now...
-Kees
[1] https://gcc.gnu.org/bugzilla/attachment.cgi?id=60692&action=diff
--
Kees Cook
Re: [RFC] [C]New syntax for the argument of counted_by attribute for C language
On Wed, Apr 02, 2025 at 09:16:47PM +, Qing Zhao wrote:
> Hi, Bill,
>
> Thanks for the summary.
>
> I think that this is good.
>
> Two more questions:
>
> 1. Shall we keep the same name of the attribute counted_by for the 2nd new
> syntax?
> Or use a new name, such as, “counted_by_exp"?
FWIW, the Linux kernel will likely need to create its own macros for all
of this stuff (just to do the compiler version testing), so there should
be no problem with different names (or same names) in the compiler API.
> I don’t have strong preference here. As mentioned by Jacub in a
> previous email, these two syntaxes can be distinguished by the number
> of arguments of the attribute.
>
> So for GCC, there should be no issue with either old name of a new name.
> However, I am not sure about CLANG. (Considering the complication with
> APPLE’s implementation)
>
> 2. The 2nd new syntax should resolve all the following new requests from
> Linux Kernel:
> 2.1 Refer to a field in the nested structure
> 2.2 Refer to globals or locals
> 2.3 Represent simple expression
> 2.4 Forward referencing
>
> Except not very sure on 2.1: refer to a field in the nested structure
>
> struct Y {
> int n;
> int other;
> }
>
> struct Z {
> struct Y y;
> int array[] __attribute__ ((counted_by(?y.n)));
> };
>
> in the above, what should be put instead of "?" to refer to the field "n" of
> the field "y" of the current object of this struct Z?
>
> Based on my understanding, with the new syntax,
>
> struct Z {
> struct Y y;
> int array[] __attribute__ ((counted_by(struct Y y, y.n)));
> };
>
> i.e, the compiler will lookup “y” inside the current structure, then
> resolving the member access operator “.” as an expression.
>
> Is this correct understanding?
I had the same question, e.g. how is this supposed to be declared:
struct Y {
int n;
int other;
}
struct Z {
int *ints __attribute__ ((counted_by(y.n)));
struct Y y;
};
--
Kees Cook
Re: [PATCH v6 0/3][Middle-end]Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
"%qD accessing %wu or more bytes at offsets "
"%s and %s overlaps %wu byte at offset %s",
"%qD accessing %wu or more bytes at offsets "
@@ -1572,14 +1574,14 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access &acs)
func, sizrange[0], offstr[0], offstr[1],
ovlsiz[0], offstr[2]);
else if (ovlsiz[1] >= 0 && ovlsiz[1] < maxobjsize.to_shwi ())
- warning_at (loc, OPT_Wrestrict,
+ warning_at (&richloc, OPT_Wrestrict,
"%qD accessing %wu or more bytes at offsets %s "
"and %s overlaps between %wu and %wu bytes "
"at offset %s",
func, sizrange[0], offstr[0], offstr[1],
ovlsiz[0], ovlsiz[1], offstr[2]);
else
- warning_at (loc, OPT_Wrestrict,
+ warning_at (&richloc, OPT_Wrestrict,
"%qD accessing %wu or more bytes at offsets %s "
"and %s overlaps %wu or more bytes at offset %s",
func, sizrange[0], offstr[0], offstr[1],
@@ -1607,14 +1609,14 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access &acs)
if (ovlsiz[1] == 1)
{
if (open_range)
- warning_n (loc, OPT_Wrestrict, sizrange[1],
+ warning_n (&richloc, OPT_Wrestrict, sizrange[1],
"%qD accessing %wu byte may overlap "
"%wu byte",
"%qD accessing %wu bytes may overlap "
"%wu byte",
func, sizrange[1], ovlsiz[1]);
else
- warning_n (loc, OPT_Wrestrict, sizrange[1],
+ warning_n (&richloc, OPT_Wrestrict, sizrange[1],
"%qD accessing %wu byte at offsets %s "
"and %s may overlap %wu byte at offset %s",
"%qD accessing %wu bytes at offsets %s "
@@ -1625,14 +1627,14 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access &acs)
}
if (open_range)
- warning_n (loc, OPT_Wrestrict, sizrange[1],
+ warning_n (&richloc, OPT_Wrestrict, sizrange[1],
"%qD accessing %wu byte may overlap "
"up to %wu bytes",
"%qD accessing %wu bytes may overlap "
"up to %wu bytes",
func, sizrange[1], ovlsiz[1]);
else
- warning_n (loc, OPT_Wrestrict, sizrange[1],
+ warning_n (&richloc, OPT_Wrestrict, sizrange[1],
"%qD accessing %wu byte at offsets %s and "
"%s may overlap up to %wu bytes at offset %s",
"%qD accessing %wu bytes at offsets %s and "
@@ -1645,14 +1647,14 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access &acs)
if (sizrange[1] >= 0 && sizrange[1] < maxobjsize.to_shwi ())
{
if (open_range)
- warning_n (loc, OPT_Wrestrict, ovlsiz[1],
+ warning_n (&richloc, OPT_Wrestrict, ovlsiz[1],
"%qD accessing between %wu and %wu bytes "
"may overlap %wu byte",
"%qD accessing between %wu and %wu bytes "
"may overlap up to %wu bytes",
func, sizrange[0], sizrange[1], ovlsiz[1]);
else
- warning_n (loc, OPT_Wrestrict, ovlsiz[1],
+ warning_n (&richloc, OPT_Wrestrict, ovlsiz[1],
"%qD accessing between %wu and %wu bytes "
"at offsets %s and %s may overlap %wu byte "
"at offset %s",
@@ -1664,7 +1666,7 @@ maybe_diag_overlap (location_t loc, gimple *call,
builtin_access &acs)
return true;
}
- warning_n (loc, OPT_Wrestrict, ovlsiz[1],
+ warning_n (&richloc, OPT_Wrestrict, ovlsiz[1],
"%qD accessing %wu or more bytes at offsets %s "
"and %s may overlap %wu byte at offset %s",
"%qD accessing %wu or more bytes at offsets %s "
--
Kees Cook
Re: [PATCH v6 0/3][Middle-end]Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
On Thu, Jun 05, 2025 at 03:52:21PM -0700, Kees Cook wrote: > On Mon, May 19, 2025 at 11:23:34AM -0700, Kees Cook wrote: > > On Fri, May 16, 2025 at 01:34:14PM +, Qing Zhao wrote: > > > Adding -fdiagnotics-details into GCC to provide more hints to the > > > end users on how the warnings come from, in order to help the user > > > to locate the exact location in source code on the specific warnings > > > due to compiler optimizations. > > > > I just needed to examine an unexpected -Wrestrict warning, and > > discovered that this patch didn't help with it, but in looking at the > > implementation details, it turned out to be trivial to expand coverage > > to include -Wrestrict, which worked for me, and got me the > > diagnostics I needed[1]. > > I found another case[1] where I didn't get detailed diagnostics, so I > tried to instrument that too, but it didn't do anything. Here's the patch > (trying to get more coverage for stringop-overflow), but I don't know > what I did wrong: I think this may be a known bug. It looks very similar to this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97490 and is somehow related to -fsanitize=kernel-address -- Kees Cook
Re: [PATCH v6 0/3][Middle-end]Provide more contexts for -Warray-bounds and -Wstringop-* warning messages
On Mon, May 19, 2025 at 11:23:34AM -0700, Kees Cook wrote:
> On Fri, May 16, 2025 at 01:34:14PM +, Qing Zhao wrote:
> > Adding -fdiagnotics-details into GCC to provide more hints to the
> > end users on how the warnings come from, in order to help the user
> > to locate the exact location in source code on the specific warnings
> > due to compiler optimizations.
>
> I just needed to examine an unexpected -Wrestrict warning, and
> discovered that this patch didn't help with it, but in looking at the
> implementation details, it turned out to be trivial to expand coverage
> to include -Wrestrict, which worked for me, and got me the
> diagnostics I needed[1].
I found another case[1] where I didn't get detailed diagnostics, so I
tried to instrument that too, but it didn't do anything. Here's the patch
(trying to get more coverage for stringop-overflow), but I don't know
what I did wrong:
diff --git a/gcc/tree-ssa-strlen.cc b/gcc/tree-ssa-strlen.cc
index c4d64132e538..6342231afba2 100644
--- a/gcc/tree-ssa-strlen.cc
+++ b/gcc/tree-ssa-strlen.cc
@@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-ssa-loop.h"
#include "tree-scalar-evolution.h"
#include "vr-values.h"
+#include "move-history-rich-location.h"
#include "gimple-range.h"
#include "tree-ssa.h"
@@ -2113,6 +2114,8 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
return;
location_t loc = gimple_or_expr_nonartificial_location (stmt, dest);
+ rich_location_with_details richloc (loc, stmt);
+
bool warned = false;
if (wi::leu_p (lenrng[0], spcrng[1]))
{
@@ -2121,11 +2124,11 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
return;
warned = (writefn
- ? warning_at (loc, OPT_Wstringop_overflow_,
+ ? warning_at (&richloc, OPT_Wstringop_overflow_,
"%qD writing one too many bytes into a region "
"of a size that depends on %",
writefn)
- : warning_at (loc, OPT_Wstringop_overflow_,
+ : warning_at (&richloc, OPT_Wstringop_overflow_,
"writing one too many bytes into a region "
"of a size that depends on %"));
}
@@ -2133,7 +2136,7 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
{
if (spcrng[0] == spcrng[1])
warned = (writefn
- ? warning_n (loc, OPT_Wstringop_overflow_,
+ ? warning_n (&richloc, OPT_Wstringop_overflow_,
lenrng[0].to_uhwi (),
"%qD writing %wu byte into a region "
"of size %wu",
@@ -2141,7 +2144,7 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
"of size %wu",
writefn, lenrng[0].to_uhwi (),
spcrng[0].to_uhwi ())
- : warning_n (loc, OPT_Wstringop_overflow_,
+ : warning_n (&richloc, OPT_Wstringop_overflow_,
lenrng[0].to_uhwi (),
"writing %wu byte into a region "
"of size %wu",
@@ -2151,7 +2154,7 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
spcrng[0].to_uhwi ()));
else
warned = (writefn
- ? warning_n (loc, OPT_Wstringop_overflow_,
+ ? warning_n (&richloc, OPT_Wstringop_overflow_,
lenrng[0].to_uhwi (),
"%qD writing %wu byte into a region "
"of size between %wu and %wu",
@@ -2159,7 +2162,7 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
"of size between %wu and %wu",
writefn, lenrng[0].to_uhwi (),
spcrng[0].to_uhwi (), spcrng[1].to_uhwi ())
- : warning_n (loc, OPT_Wstringop_overflow_,
+ : warning_n (&richloc, OPT_Wstringop_overflow_,
lenrng[0].to_uhwi (),
"writing %wu byte into a region "
"of size between %wu and %wu",
@@ -2170,13 +2173,13 @@ strlen_pass::maybe_warn_overflow (gimple *stmt, bool
call_lhs, tree len,
}
else if (spcrng[0] == spcrng[1])
warned = (writefn
- ? warning_at (loc, OPT_Wstringop_overflow_,
+ ? warning_at (&richl
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
On Thu, May 25, 2023 at 04:14:47PM +, Qing Zhao wrote: > This patch set introduces a new attribute "element_count" to annotate bounds > for C99 flexible array member. Thank you for this work! I'm really excited to start using it in the Linux kernel. I'll give this a spin, but I know you've already been testing this series against the test cases I created earlier, so I don't expect any problems. :) One bike-shedding note: with the recent "-fbounds-safety" RFC posted for LLVM, we may want to consider renaming "element_count" to "counted_by": https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/ Thanks again! -Kees -- Kees Cook
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
On Thu, May 25, 2023 at 04:14:47PM +, Qing Zhao wrote: > GCC will pass the number of elements info from the attached attribute to both > __builtin_dynamic_object_size and bounds sanitizer to check the out-of-bounds > or dynamic object size issues during runtime for flexible array members. > > This new feature will provide nice protection to flexible array members (which > currently are completely ignored by both __builtin_dynamic_object_size and > bounds sanitizers). Testing went pretty well, though I think I found some bdos issues: - some things that bdos can't know the size of, and correctly returned SIZE_MAX in the past, now thinks are 0-sized. - while bdos correctly knows the size of an element_count-annotated flexible array, it doesn't know the size of the containing object (i.e. it returns SIZE_MAX). Also, I think I found a precedence issue: - if both __alloc_size and 'element_count' are in use, the _smallest_ of the two is what I would expect to be enforced by the sanitizer and reported by __bdos. As is, alloc_size appears to be used when it is available, regardless of what 'element_count' shows. I've updated my test cases to show it more clearly, but here is the before/after: GCC 13 (correctly does not implement "element_count"): $ ./array-bounds 2>&1 | grep -v ^'#' TAP version 13 1..12 ok 1 global.fixed_size_seen_by_bdos ok 2 global.fixed_size_enforced_by_sanitizer ok 3 global.unknown_size_unknown_to_bdos ok 4 global.unknown_size_ignored_by_sanitizer ok 5 global.alloc_size_seen_by_bdos ok 6 global.alloc_size_enforced_by_sanitizer not ok 7 global.element_count_seen_by_bdos not ok 8 global.element_count_enforced_by_sanitizer not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer ToT GCC + this element_count series: $ ./array-bounds 2>&1 | grep -v ^'#' TAP version 13 1..12 ok 1 global.fixed_size_seen_by_bdos ok 2 global.fixed_size_enforced_by_sanitizer not ok 3 global.unknown_size_unknown_to_bdos not ok 4 global.unknown_size_ignored_by_sanitizer ok 5 global.alloc_size_seen_by_bdos ok 6 global.alloc_size_enforced_by_sanitizer not ok 7 global.element_count_seen_by_bdos ok 8 global.element_count_enforced_by_sanitizer not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer Test suite is here: https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c -- Kees Cook
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
gt;array. > > I think that this should be considered as an user error, and the > documentation of the attribute should include > this requirement. (In the LLVM’s RFC, such requirement was included in the > programing model: > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18) > > We can add a new warning option -Wcounted-by to report such user error if > needed. > > What’s your opinion on this? I think it is correct to allow mismatch between allocation and counted_by as long as only the least of the two is used. This may be desirable in a few situations. One example would be a large allocation that is slowly filled up by the program. I.e. the counted_by member is slowly increased during runtime (but not beyond the true allocation size). Of course allocation size is only available in limited situations, so the loss of that info is fine: we have counted_by for everything else. -- Kees Cook
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
On Mon, Jul 17, 2023 at 09:17:48PM +, Qing Zhao wrote:
>
> > On Jul 13, 2023, at 4:31 PM, Kees Cook wrote:
> >
> > In the bug, the problem is that "p" isn't known to be allocated, if I'm
> > reading that correctly?
>
> I think that the major point in PR109557
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557):
>
> for the following pointer p.3_1,
>
> p.3_1 = p;
> _2 = __builtin_object_size (p.3_1, 0);
>
> Question: why the size of p.3_1 cannot use the TYPE_SIZE of the pointee of p
> when the TYPE_SIZE can be determined at compile time?
>
> Answer: From just knowing the type of the pointee of p, the compiler cannot
> determine the size of the object.
Why is that? "p" points to "struct P", which has a fixed size. There
must be an assumption somewhere that a pointer is allocated, otherwise
__bos would almost never work?
> Therefore the bug has been closed.
>
> In your following testing 5:
>
> > I'm not sure this is a reasonable behavior, but
> > let me get into the specific test, which looks like this:
> >
> > TEST(counted_by_seen_by_bdos)
> > {
> >struct annotated *p;
> >int index = MAX_INDEX + unconst;
> >
> >p = alloc_annotated(index);
> >
> >REPORT_SIZE(p->array);
> > /* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array));
> >/* Check array size alone. */
> > /* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1), SIZE_MAX);
> > /* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->foo *
> > sizeof(*p->array));
> >/* Check check entire object size. */
> > /* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX);
> > /* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->foo
> > * sizeof(*p->array));
> > }
> >
> > Test 5 should pass as well, since, again, p can be examined. Passing p
> > to __bdos implies it is allocated and the __counted_by annotation can be
> > examined.
>
> Since the call to the routine “alloc_annotated" cannot be inlined, GCC does
> not see any allocation calls for the pointer p.
Correct.
> At the same time, due to the same reason as PR109986, GCC cannot determine
> the size of the object by just knowing the TYPE_SIZE
> of the pointee of p.
So the difference between test 3 and test 5 is that "p" is explicitly
dereferenced to find "array", and therefore an assumption is established
that "p" is allocated?
> So, this is exactly the same issue as PR109557. It’s an existing behavior
> per the current __buitlin_object_size algorithm.
> I am still not very sure whether the situation in PR109557 can be improved or
> not, but either way, it’s a separate issue.
Okay, so the issue is one of object allocation visibility (or
assumptions there in)?
> Please see the new testing case I added for PR109557, you will see that the
> above case 5 is a similar case as the new testing case in PR109557.
I will ponder this a bit more to see if I can come up with a useful
test case to replace the part from "test 5" above.
> >
> > If "return p->array[index];" would be caught by the sanitizer, then
> > it follows that __builtin_dynamic_object_size(p, 1) must also know the
> > size. i.e. both must examine "p" and its trailing flexible array and
> > __counted_by annotation.
> >
> >>
> >> 2. The common issue for the failed testing 3, 4, 9, 10 is:
> >>
> >> for the following annotated structure:
> >>
> >>
> >> struct annotated {
> >>unsigned long flags;
> >>size_t foo;
> >>int array[] __attribute__((counted_by (foo)));
> >> };
> >>
> >>
> >> struct annotated *p;
> >> int index = 16;
> >>
> >> p = malloc(sizeof(*p) + index * sizeof(*p->array)); // allocated real
> >> size
> >>
> >> p->foo = index + 2; // p->foo was set by a different value than the real
> >> size of p->array as in test 9 and 10
> >
> > Right, tests 9 and 10 are checking that the _smallest_ possible value of
> > the array is used. (There are two sources of information: the allocation
> > size and the size calculated by counted_by. The smaller of the two
> > should be used when both are available.)
>
> The counted_by attribute is used to annotate a Flexible array member on how
> many elements it will have.
> However, if this information can not accurately reflect the real number of
>
Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)
On Mon, Jul 31, 2023 at 08:14:42PM +, Qing Zhao wrote: > /* In general, Due to type casting, the type for the pointee of a pointer >does not say anything about the object it points to, >So, __builtin_object_size can not directly use the type of the pointee >to decide the size of the object the pointer points to. > >there are only two reliable ways: >A. observed allocations (call to the allocation functions in the routine) >B. observed accesses (read or write access to the location of the > pointer points to) > >that provide information about the type/existence of an object at >the corresponding address. > >for A, we use the "alloc_size" attribute for the corresponding allocation >functions to determine the object size; > >For B, we use the SIZE info of the TYPE attached to the corresponding > access. >(We treat counted_by attribute as a complement to the SIZE info of the TYPE > for FMA) > >The only other way in C which ensures that a pointer actually points >to an object of the correct type is 'static': > >void foo(struct P *p[static 1]); > >See https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624814.html >for more details. */ This is a great explanation; thank you! In the future I might want to have a new builtin that will allow a program to query a pointer when neither A nor B have happened. But for the first version of the __counted_by infrastructure, the above limitations seen fine. For example, maybe __builtin_counted_size(p) (which returns sizeof(*p) + sizeof(*p->flex_array_member) * p->counted_by_member). Though since there might be multiple flex array members, maybe this can't work. :) -Kees -- Kees Cook
Re: One question on the source code of tree-object-size.cc
ace than size of the
>struct fix. Then what's the correct behavior we expect
>the __builtin_object_size should have for the following?
> */
>
> static struct fix * noinline alloc_buf_less ()
> {
> struct fix *p;
> p = malloc(sizeof (struct fix) - SIZE_BUMP * sizeof (int));
>
> /*when checking the observed access p->array, we have info on both
> observered allocation and observed access,
> A. from observed allocation (alloc_size): (LENGTH - SIZE_BUMP) * sizeof
> (int)
> B. from observed access (TYPE): LENGTH * sizeof (int)
>*/
>
> /* for MAXIMUM size in the whole object: currently, GCC always used the A.
> */
> expect(__builtin_object_size(p->array, 0), (LENGTH - SIZE_BUMP) *
> sizeof(int));
ok: __builtin_object_size(p->array, 0) == 20
My brain just melted a little, as this is now an under-sized instance of
"p", so we have an incomplete allocation. (I would expect -Warray-bounds
to yell very loudly for this.) But, technically, yes, this looks like
the right calculation.
>
> /* for MAXIMUM size in the sub-object: currently, GCC chose the smaller
> one among these two: B. */
> expect(__builtin_object_size(p->array, 1), (LENGTH - SIZE_BUMP) *
> sizeof(int));
ok: __builtin_object_size(p->array, 1) == 20
Given the under-allocation, yes, this seems correct to me, though,
again, I would expect a compile-time warning. (Or at least, I've seen
-Warray-bounds yell about this kind of thing in the past.)
-Kees
--
Kees Cook
Re: [GCC13][Patch][V2][1/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836
On Tue, Jul 19, 2022 at 02:09:46PM +, Qing Zhao wrote: > [...] > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc > index c8d96723f4c..10d16532f0d 100644 > --- a/gcc/c-family/c-attribs.cc > +++ b/gcc/c-family/c-attribs.cc > @@ -101,6 +101,8 @@ static tree handle_special_var_sec_attribute (tree *, > tree, tree, int, bool *); > static tree handle_aligned_attribute (tree *, tree, tree, int, bool *); > static tree handle_warn_if_not_aligned_attribute (tree *, tree, tree, > int, bool *); > +static tree handle_strict_flex_array_attribute (tree *, tree, tree, > + int, bool *); Something mangled these patches -- leading blank characters got dropped? -- Kees Cook
Re: [GCC13][Patch][V2][1/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836
On Thu, Jul 28, 2022 at 07:26:57AM +, Richard Biener wrote:
> On Tue, 19 Jul 2022, Qing Zhao wrote:
> > [...]
> > +@cindex @code{strict_flex_array} variable attribute
> > +@item strict_flex_array (@var{level})
> > +The @code{strict_flex_array} attribute should be attached to the trailing
> > +array field of a structure. It specifies the level of strictness of
> > +treating the trailing array field of a structure as a flexible array
> > +member. @var{level} must be an integer betwen 0 to 3.
> > +
> > +@var{level}=0 is the least strict level, all trailing arrays of structures
> > +are treated as flexible array members. @var{level}=3 is the strictest
> > level,
> > +only when the trailing array is declared as a flexible array member per C99
> > +standard onwards ([]), it is treated as a flexible array member.
>
> How is level 3 (thus -fstrict-flex-array) interpreted when you specify
> -std=c89? How for -std=gnu89?
To me, it makes sense that either c99 is required (most sane to me)
or it would disable flexible arrays entirely (seems an unlikely combo to
be useful).
>
> > +
> > +There are two more levels in between 0 and 3, which are provided to support
> > +older codes that use GCC zero-length array extension ([0]) or one-size
> > array
> > +as flexible array member ([1]):
> > +When @var{level} is 1, the trailing array is treated as a flexible array
> > member
> > +when it is declared as either "[]", "[0]", or "[1]";
> > +When @var{level} is 2, the trailing array is treated as a flexible array
> > member
> > +when it is declared as either "[]", or "[0]".
>
> Given the above does adding level 2 make sense given that [0] is a GNU
> extension?
Level 1 removes the general "all trailing arrays are flex arrays" logic, but
allows the 2 common "historical" fake flex array styles ("[1]" and "[0]").
Level 2 additionally removes the "[1]" style.
Level 3 additionally removes the "[0]" style.
I don't understand how "[0]" being a GNU extension matters here for
level 2 -- it's dropping "[1]". And for level 3, the point is to defang
the GNU extension of "[0]" to no longer mean "flexible array", and
instead only mean "zero sized member" (as if it were something like
"struct { } no_size;").
Note that for the Linux kernel, we only care about level 3, but could
make do with level 2. We need to purge all the "fake" flexible array usage
so we can start building a sane set of behaviors around array bounds
that are reliably introspectable.
As a related bit of feature creep, it would be great to expose something
like __builtin_has_flex_array_p() so FORTIFY could do a better job
filtering __builtin_object_size() information.
Given:
struct inside {
int foo;
int bar;
unsigned long items[];
};
struct outside {
int a;
int b;
struct inside inner;
};
The follow properties are seen within, for example:
void stuff(struct outside *outer, struct inside *inner)
{
...
}
__builtin_object_size(&outer->inner, 1) == 8
__builtin_object_size(inner, 1) == -1
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832)
So things like FORTIFY misfire on &outer->inner, as it's _not_ actually
8 bytes -- it has a potential trailing flex array.
If it could be introspected better, FORTIFY could check for the flex
array. For example, instead of using the inconsistent __bos(ptr, 1) for
finding member sizes, it could do something like:
#define __member_size(ptr) \
(__builtin_has_flex_array_p(ptr) ? -1 : \
__builtin_object_size(ptr, 1))
--
Kees Cook
Re: [GCC13][Patch][V2][2/2]Add a new option -fstrict-flex-array[=n] and attribute strict_flex_array(n) and use it in PR101836
On Tue, Jul 19, 2022 at 02:11:19PM +, Qing Zhao wrote: > From a09f39ded462611286a44d9e8273de8342673ba2 Mon Sep 17 00:00:00 2001 > From: Qing Zhao > Date: Mon, 18 Jul 2022 18:12:26 + > Subject: [PATCH 2/2] Use new flag DECL_NOT_FLEXARRAY in __builtin_object_size > [PR101836] > > Use new flag DECL_NOT_FLEXARRAY to determine whether the trailing array > of a structure is flexible array member in __builtin_object_size. FWIW, with these patches I've done builds of the Linux kernel using the ...=3 level, and things are looking correct on our end. (There are, as expected, many places in Linux that need to be fixed, and that work is on-going, guided by this option's results.) -Kees -- Kees Cook
Re: One question on the source code of tree-object-size.cc
On August 3, 2023 10:34:24 AM PDT, Qing Zhao wrote:
>One thing I need to point out first is, currently, even for regular fixed size
>array in the structure,
>We have this same issue, for example:
>
>#define LENGTH 10
>
>struct fix {
> size_t foo;
> int array[LENGTH];
>};
>
>…
>int main ()
>{
> struct fix *p;
> p = alloc_buf_more ();
>
> expect(__builtin_object_size(p->array, 1), LENGTH * sizeof(int));
> expect(__builtin_object_size(p->array, 0), -1);
>}
>
>Currently, for __builtin_object_size(p->array, 0), GCC return UNKNOWN for it.
>This is not a special issue for flexible array member.
Is this true with -fstrict-flex-arrays=3 ?
-Kees
>
>Qing
>
>
>On Aug 3, 2023, at 1:19 PM, Siddhesh Poyarekar wrote:
>>
>> On 2023-08-03 12:43, Qing Zhao wrote:
>>>> Surely we could emit that for __bdos(q->array, 0) though, couldn't we?
>>> For __bdos(q->array, 0), we only have the access info for the sub-object
>>> q->array, we can surely decide the size of the sub-object q->array, but we
>>> still cannot
>>> decide the whole object that is pointed by q (the same reason as above),
>>> right?
>>
>> It's tricky, I mean we could assume p to be a valid object due to the
>> dereference and hence assume that q->foo is also valid and that there's at
>> least sizeof(*q) + q->foo * sizeof (q->array) bytes available. The question
>> then is whether q could be pointing to an element of an array of `struct
>> annotated`. Could we ever have a valid array of such structs that have a
>> flex array at the end? Wouldn't it always be a single object?
>>
>> In fact for all pointers to such structs with a flex array at the end, could
>> we always assume that it is a single object and never part of an array, and
>> hence return sizeof()?
>>
>> Thanks,
>> Sid
>
--
Kees Cook
Re: One question on the source code of tree-object-size.cc
On Thu, Aug 03, 2023 at 07:55:54PM +, Qing Zhao wrote:
>
>
> > On Aug 3, 2023, at 1:51 PM, Kees Cook wrote:
> >
> > On August 3, 2023 10:34:24 AM PDT, Qing Zhao wrote:
> >> One thing I need to point out first is, currently, even for regular fixed
> >> size array in the structure,
> >> We have this same issue, for example:
> >>
> >> #define LENGTH 10
> >>
> >> struct fix {
> >> size_t foo;
> >> int array[LENGTH];
> >> };
> >>
> >> …
> >> int main ()
> >> {
> >> struct fix *p;
> >> p = alloc_buf_more ();
> >>
> >> expect(__builtin_object_size(p->array, 1), LENGTH * sizeof(int));
> >> expect(__builtin_object_size(p->array, 0), -1);
> >> }
> >>
> >> Currently, for __builtin_object_size(p->array, 0), GCC return UNKNOWN for
> >> it.
> >> This is not a special issue for flexible array member.
> >
> > Is this true with -fstrict-flex-arrays=3 ?
>
> Yes.
Okay, right, I understand now -- it doesn't see the allocation, therefore
max size is unknown. Sounds good.
-Kees
--
Kees Cook
Re: One question on the source code of tree-object-size.cc
On Thu, Aug 03, 2023 at 09:31:24PM +, Qing Zhao wrote:
> So, the basic question is:
>
> Given the following:
>
> struct fix {
> int others;
> int array[10];
> }
>
> extern struct fix * alloc_buf ();
>
> int main ()
> {
> struct fix *p = alloc_buf ();
> __builtin_object_size(p->array,0) == ?
> }
>
> Given p->array, can the compiler determine that p points to an object that
> has TYPE struct fix?
>
> If the answer is YES, then the current__builtin_object_size algorithm can be
> improved to determine __builtin_object_size(p->array, 0) with the TYPE of
> the struct fix.
I think it is fine to leave __bos(..., 0) as-is. From the Linux kernel's
use of __bos, we are almost exclusively only interesting the mode 1, not
node 0. :)
--
Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Aug 04, 2023 at 07:44:28PM +, Qing Zhao wrote:
> This is the 2nd version of the patch, per our discussion based on the
> review comments for the 1st version, the major changes in this version
> are:
Thanks for the update!
>
> 1. change the name "element_count" to "counted_by";
> 2. change the parameter for the attribute from a STRING to an
> Identifier;
> 3. Add logic and testing cases to handle anonymous structure/unions;
> 4. Clarify documentation to permit the situation when the allocation
> size is larger than what's specified by "counted_by", at the same time,
> it's user's error if allocation size is smaller than what's specified by
> "counted_by";
> 5. Add a complete testing case for using counted_by attribute in
> __builtin_dynamic_object_size when there is mismatch between the
> allocation size and the value of "counted_by", the expecting behavior
> for each case and the explanation on why in the comments.
All the "normal" test cases I have are passing; this is wonderful! :)
I'm still seeing unexpected situations when I've intentionally set
counted_by to be smaller than alloc_size, but I assume it's due to not
yet having the patch you mention below.
> As discussed, I plan to add two more separate patch sets after this initial
> patch set is approved and committed.
>
> set 1. A new warning option and a new sanitizer option for the user error
>when the allocation size is smaller than the value of "counted_by".
> set 2. An improvement to __builtin_dynamic_object_size for the following
>case:
>
> struct A
> {
> size_t foo;
> int array[] __attribute__((counted_by (foo)));
> };
>
> extern struct fix * alloc_buf ();
>
> int main ()
> {
> struct fix *p = alloc_buf ();
> __builtin_object_size(p->array, 0) == sizeof(struct A) + p->foo * sizeof(int);
> /* with the current algorithm, it’s UNKNOWN */
> __builtin_object_size(p->array, 2) == sizeof(struct A) + p->foo * sizeof(int);
> /* with the current algorithm, it’s UNKNOWN */
> }
Should the above be bdos instead of bos?
> Bootstrapped and regression tested on both aarch64 and X86, no issue.
I've updated the Linux kernel's macros for the name change and done
build tests with my first pass at "easy" cases for adding counted_by:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/counted_by&id=adc5b3cb48a049563dc673f348eab7b6beba8a9b
Everything is working as expected. :)
-Kees
--
Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Tue, Aug 08, 2023 at 04:54:46PM +0200, Martin Uecker wrote:
>
>
> I am sure this has been discussed before, but seeing that you
> test for a specific formula, let me point out the following:
>
> There at least three different size expression which could
> make sense. Consider
>
> short foo { int a; short b; char t[]; };
>
> Most people seem to use
>
> sizeof(struct foo) + N * sizeof(foo->t);
I think this contains a typo. The above should be:
sizeof(struct foo) + N * sizeof(*foo->t);
> which for N == 3 yields 11 bytes on x86-64 because the formula
> adds the padding of the original struct. There is an example
> in the C standard that uses this formula.
>
>
> But he minimum size of an object which stores N elements is
>
> max(sizeof (struct s), offsetof(struct s, t[n]))
>
> which is 9 bytes.
>
> This is what clang uses for statically allocated objects with
> initialization, while GCC uses the rule above (11 bytes). But
> bdos / bos then returns the smaller size of 9 which is a bit
> confusing.
>
> https://godbolt.org/z/K1hvaK1ns
And to add to the mess here, Clang used to get this wrong for statically
initialized flexible array members (returning 8):
https://godbolt.org/z/Tee96dazs
But that was fixed recently when I noticed it a few weeks ago.
> https://github.com/llvm/llvm-project/issues/62929
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109956
>
>
> Then there is also the size of a similar array where the FAM
> is replaced with an array of static size:
>
> struct foo { int a; short b; char t[3]; };
>
> This would make the most sense to me, but it has 12 bytes
> because the padding is according to the usual alignment
> rules.
Hm, in my thinking, FAMs are individually padded, so since they don't
"count" to the struct size, I think this is "as expected".
Note that the Linux kernel explicitly chooses to potentially over-estimate
for FAM allocations since there has been inconsistent sizes used depend
on the style of the prior fake FAM usage, the use of unions, etc.
i.e. our macro is, effectively:
#define struct_size(ptr, member, count) \
(sizeof(*ptr) + (count) * sizeof(*ptr->member))
since the other case can lead to truncated sizes:
#define struct_size(ptr, member, count) \
(offsetof(typeof(*ptr), member) + (count) * sizeof(*ptr->member))
struct foo {
int a, b, c;
int count;
union {
struct {
char small;
char char_fam[];
};
struct {
int large;
int int_fam[];
};
};
};
https://godbolt.org/z/b1v74Mzhd
i.e.:
struct_size(x, char_fam, 1) < sizeof(*x)
so accessing "large" fails, etc. Yes, it's all horrible, but we have to
deal with this kind of thing in the kernel. :(
--
Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Mon, Aug 07, 2023 at 04:33:13PM +, Qing Zhao wrote: > What’s the testing case for the one that failed? > If it’s > > __builtin_dynamic_object_size(p->array, 0/2) without the allocation > information in the routine, > then with the current algorithm, gcc cannot deduce the size for the whole > object. > > If not such case, let me know. I found some more bugs in my tests (now fixed), but I'm left with a single failure case, which is think it going to boil down to pointer/pointee issue we discussed earlier. Using my existing testing tool: https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c I see this error with the "counted_by_seen_by_bdos" case: Expected __builtin_dynamic_object_size(p, 1) (18446744073709551615) == sizeof(*p) + p->count * sizeof(*p->array) (80) A reduced view of the code is: struct annotated *p; int index = MAX_INDEX + unconst; p = alloc_annotated(index); EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->count * sizeof(*p->array)); It looks like __bdos will not use the __counted_by information from the pointee if the argument is only the pointer. i.e. this test works: EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->count * sizeof(*p->array)); However, I thought if any part of the pointee was used (e.g. p->count, p->array), GCC would be happy to start using the pointee details? And, again, for the initial version at this feature, I'm fine with this failing test being declared not a valid test. :) But I'll need some kind of builtin that can correctly interrogate a pointer to find the full runtime size with the assumption that pointer is valid, but that can come later. And as a side note, I am excited to see the very correct warnings for bad (too late) assignment of the __counted_by member: p->array[0] = 0; p->count = 1; array-bounds.c: In function 'invalid_assignment_order': array-bounds.c:366:17: warning: '*p.count' is used uninitialized [-Wuninitialized] 366 | p->array[0] = 0; | ^~~ Yay! :) -Kees -- Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Fri, Aug 04, 2023 at 07:44:28PM +, Qing Zhao wrote:
> This is the 2nd version of the patch, per our discussion based on the
> review comments for the 1st version, the major changes in this version
I've been using Coccinelle to find and annotate[1] structures (193 so
far...), and I've encountered 2 cases of GCC internal errors. I'm working
on a minimized test case, but just in case these details are immediately
helpful, here's what I'm seeing:
../drivers/net/wireless/ath/wcn36xx/smd.c: In function
'wcn36xx_smd_rsp_process':
../drivers/net/wireless/ath/wcn36xx/smd.c:3299:5: error: incorrect sharing of
tree nodes
3299 | int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
| ^~~
MEM[(struct wcn36xx_hal_ind_msg *)_96]
_15 = &MEM[(struct wcn36xx_hal_ind_msg *)_96].msg;
during GIMPLE pass: objsz
../drivers/net/wireless/ath/wcn36xx/smd.c:3299:5: internal compiler error:
verify_gimple failed
0xfe97fd verify_gimple_in_cfg(function*, bool, bool)
../../../../gcc/gcc/tree-cfg.cc:5646
0xe84894 execute_function_todo
../../../../gcc/gcc/passes.cc:2088
0xe84dee execute_todo
../../../../gcc/gcc/passes.cc:2142
The associated struct is:
struct wcn36xx_hal_ind_msg {
struct list_head list;
size_t msg_len;
u8 msg[] __counted_by(msg_len);
};
And:
../drivers/usb/gadget/function/f_fs.c: In function '__ffs_epfile_read_data':
../drivers/usb/gadget/function/f_fs.c:900:16: error: incorrect sharing of tree
nodes
900 | static ssize_t __ffs_epfile_read_data(struct ffs_epfile *epfile,
|^~
MEM[(struct ffs_buffer *)_67]
_5 = &MEM[(struct ffs_buffer *)_67].storage;
during GIMPLE pass: objsz
../drivers/usb/gadget/function/f_fs.c:900:16: internal compiler error:
verify_gimple failed
0xfe97fd verify_gimple_in_cfg(function*, bool, bool)
../../../../gcc/gcc/tree-cfg.cc:5646
0xe84894 execute_function_todo
../../../../gcc/gcc/passes.cc:2088
0xe84dee execute_todo
../../../../gcc/gcc/passes.cc:2142
with:
struct ffs_buffer {
size_t length;
char *data;
char storage[] __counted_by(length);
};
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
--
Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Wed, Aug 16, 2023 at 10:31:30PM -0700, Kees Cook wrote:
> On Fri, Aug 04, 2023 at 07:44:28PM +, Qing Zhao wrote:
> > This is the 2nd version of the patch, per our discussion based on the
> > review comments for the 1st version, the major changes in this version
>
> I've been using Coccinelle to find and annotate[1] structures (193 so
> far...), and I've encountered 2 cases of GCC internal errors. I'm working
> on a minimized test case, but just in case these details are immediately
> helpful, here's what I'm seeing:
Okay, I got it minimized:
$ cat poc.c
struct a {
unsigned long c;
char d[] __attribute__((__counted_by__(c)));
} *b;
void f(long);
void e(void) {
long g = __builtin_dynamic_object_size(b->d, 1);
f(g);
}
$ gcc -O2 -c -o /dev/null poc.c
poc.c: In function 'e':
poc.c:8:6: error: incorrect sharing of tree nodes
8 | void e(void) {
| ^
*b.0_1
_2 = &b.0_1->d;
during GIMPLE pass: objsz
poc.c:8:6: internal compiler error: verify_gimple failed
0xfe97fd verify_gimple_in_cfg(function*, bool, bool)
../../../../gcc/gcc/tree-cfg.cc:5646
0xe84894 execute_function_todo
../../../../gcc/gcc/passes.cc:2088
0xe84dee execute_todo
../../../../gcc/gcc/passes.cc:2142
--
Kees Cook
Re: [V2][PATCH 0/3] New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)
On Thu, Aug 17, 2023 at 01:44:42PM +, Qing Zhao wrote: > Thanks for the testing case. > Yes, I noticed this issue too, and already fixed it in my private branch. > > With the latest patch, the compilation has no issue: > [opc@qinzhao-ol8u3-x86 108896]$ sh t > /home/opc/Install/latest-d/bin/gcc -O2 -c -o /dev/null bug.c > [opc@qinzhao-ol8u3-x86 108896]$ Great! Thanks. I look forward to v3. For now I'll leave off these 2 annotations in my kernel builds. :) -Kees -- Kees Cook
Re: [RFC/RFT,V2 0/3] Add compiler support for Kernel Control Flow Integrity
On Sat, Mar 25, 2023 at 01:11:14AM -0700, Dan Li wrote: > This series of patches is mainly used to support the control flow > integrity protection of the linux kernel [1], which is similar to > -fsanitize=kcfi in clang 16.0 [2,3]. > > Any suggestion please let me know :). Hi Dan, It's been a couple months, and I didn't see any other feedback on this proposal. I was curious what the status of this work is. Are you able to attend GNU Cauldron[1] this year? I'd love to see this get some traction in GCC. Thanks! -Kees [1] https://gcc.gnu.org/wiki/cauldron2023 -- Kees Cook
Re: [V4][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size
On Fri, Feb 24, 2023 at 06:35:03PM +, Qing Zhao wrote: > Hi, Joseph and Richard, > > Could you please review this patch and let me know whether it’s ready > for committing into GCC13? > > The fix to Bug PR101832 is an important patch for kernel security > purpose. it's better to be put into GCC13. Hi! This series tests well for me. Getting this landed would be very nice for the Linux kernel. :) Are there any additional review notes for it, or is it ready to land? Thanks! -Kees -- Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
(please keep me in CC, I'm not subscribed...)
On Thu Feb 18, 2021 Qing Zhao said:
> Initialize automatic variables with new first class option
> -ftrivial-auto-var-init=[uninitialized|pattern|zero]
Yay! I'm really excited to see this. Thank you for working on
it! I've built GCC with this applied, and it works out of the box
for a Linux kernel build, which correctly detects the availability
of -ftrivial-auto-var-init=[pattern|zero] for the respective
CONFIG_INIT_STACK_ALL_PATTERN and CONFIG_INIT_STACK_ALL_ZERO options.
The output from the kernel's CONFIG_TEST_STACKINIT module shows coverage
for most uninitialized cases. Yay! :)
It looks like there is still some issues with padding and pre-case
switch variables. Here's the test output, FWIW:
test_stackinit: u8_zero ok
test_stackinit: u16_zero ok
test_stackinit: u32_zero ok
test_stackinit: u64_zero ok
test_stackinit: char_array_zero ok
test_stackinit: small_hole_zero ok
test_stackinit: big_hole_zero ok
test_stackinit: trailing_hole_zero ok
test_stackinit: packed_zero ok
test_stackinit: small_hole_dynamic_partial ok
test_stackinit: big_hole_dynamic_partial ok
test_stackinit: trailing_hole_dynamic_partial ok
test_stackinit: packed_dynamic_partial ok
test_stackinit: small_hole_static_partial ok
test_stackinit: big_hole_static_partial ok
test_stackinit: trailing_hole_static_partial ok
test_stackinit: packed_static_partial ok
test_stackinit: small_hole_static_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_static_all FAIL (uninit bytes: 61)
test_stackinit: trailing_hole_static_all FAIL (uninit bytes: 7)
test_stackinit: packed_static_all ok
test_stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
test_stackinit: big_hole_dynamic_all FAIL (uninit bytes: 61)
test_stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7)
test_stackinit: packed_dynamic_all ok
test_stackinit: small_hole_runtime_partial ok
test_stackinit: big_hole_runtime_partial ok
test_stackinit: trailing_hole_runtime_partial ok
test_stackinit: packed_runtime_partial ok
test_stackinit: small_hole_runtime_all ok
test_stackinit: big_hole_runtime_all ok
test_stackinit: trailing_hole_runtime_all ok
test_stackinit: packed_runtime_all ok
test_stackinit: u8_none ok
test_stackinit: u16_none ok
test_stackinit: u32_none ok
test_stackinit: u64_none ok
test_stackinit: char_array_none ok
test_stackinit: switch_1_none FAIL (uninit bytes: 8)
test_stackinit: switch_2_none FAIL (uninit bytes: 8)
test_stackinit: small_hole_none ok
test_stackinit: big_hole_none ok
test_stackinit: trailing_hole_none ok
test_stackinit: packed_none ok
test_stackinit: user ok
test_stackinit: failures: 8
The kernel's test for this is a mess[1] of macros I used to avoid losing
my sanity from cut/pasting, but it makes the tests hard to read. To
break it out, the failing cases are due to padding, as seen with the
"test_small_hole", "test_big_hole", and "test_trailing_hole" structures:
/* Simple structure with padding likely to be covered by compiler. */
struct test_small_hole {
size_t one;
char two;
/* 3 byte padding hole here. */
int three;
unsigned long four;
};
/* Try to trigger unhandled padding in a structure. */
struct test_aligned {
u32 internal1;
u64 internal2;
} __aligned(64);
struct test_big_hole {
u8 one;
u8 two;
u8 three;
/* 61 byte padding hole here. */
struct test_aligned four;
} __aligned(64);
struct test_trailing_hole {
char *one;
char *two;
char *three;
char four;
/* "sizeof(unsigned long) - 1" byte padding hole here. */
};
They fail when they're statically initialized (either fully or
partially), for example:
struct test_..._hole instance = { .two = ..., };
or
struct test_..._hole instance = { .one = ...,
.two = ...,
.three = ...,
.four = ...,
};
The last case is for switch variables outside of case statements, like
"var" here:
switch (path) {
unsigned long var;
case ..:
...
case ..:
...
...
}
I'm really looking forward to having this available. Thanks again!
-Kees
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/test_stackinit.c
--
Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Feb 25, 2021 at 12:15:01PM -0600, Qing Zhao wrote:
> > On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
> > [...]
> > test_stackinit: trailing_hole_none ok
> > test_stackinit: packed_none ok
> > test_stackinit: user ok
> > test_stackinit: failures: 8
>
> Does the above testing include “pattern initialization” in addition to “zero
> initialization”?
This was from the zero-init case. I've just tested pattern-init now and
it actually crashes GCC. I minimized the test case to this:
$ cat main.i
a() { char b[1]; }
$ gcc -ftrivial-auto-var-init=pattern -c /dev/null main.i
main.i:1:1: warning: return type defaults to ‘int’ [-Wimplicit-int]
1 | a() { char b[1]; }
| ^
main.i: In function ‘a’:
main.i:1:12: internal compiler error: in gimplify_init_ctor_eval, at
gimplify.c:4873
1 | a() { char b[1]; }
|^
0x69740d gimplify_init_ctor_eval
../../../gcc/gcc/gimplify.c:4873
0xb5ac8f gimplify_init_constructor
../../../gcc/gcc/gimplify.c:5320
0xb6b68a gimplify_modify_expr
../../../gcc/gcc/gimplify.c:5952
0xb533ba gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14262
0xb56b26 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb68e6e gimplify_and_add(tree_node*, gimple**)
../../../gcc/gcc/gimplify.c:489
0xb68e6e gimple_add_init_for_auto_var
../../../gcc/gcc/gimplify.c:1892
0xb68e6e gimplify_decl_expr
../../../gcc/gcc/gimplify.c:2010
0xb53bd6 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14459
0xb56b26 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb5727d gimplify_bind_expr
../../../gcc/gcc/gimplify.c:1421
0xb536f0 gimplify_expr(tree_node**, gimple**, gimple**, bool (*)(tree_node*),
int)
../../../gcc/gcc/gimplify.c:14463
0xb6ccc9 gimplify_stmt(tree_node**, gimple**)
../../../gcc/gcc/gimplify.c:7056
0xb6ccc9 gimplify_body(tree_node*, bool)
../../../gcc/gcc/gimplify.c:15498
0xb6d0ed gimplify_function_tree(tree_node*)
../../../gcc/gcc/gimplify.c:15652
0x9ae8d7 cgraph_node::analyze()
../../../gcc/gcc/cgraphunit.c:670
0x9b13a7 analyze_functions
../../../gcc/gcc/cgraphunit.c:1233
0x9b1f9d symbol_table::finalize_compilation_unit()
../../../gcc/gcc/cgraphunit.c:2511
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
> > struct test_..._hole instance = { .two = ..., };
> >
> > or
> >
> > struct test_..._hole instance = { .one = ...,
> > .two = ...,
> > .three = ...,
> > .four = ...,
> >
> > };
>
> So, when the structure variables are not statically initialized, all the
> paddings are initialized correctly by the compiler?
For the zero case, yes. (Usually such happen via copies into stack from from
.rodata
sections, or accidentally from already-initialized copies.)
> In the current implementation, when the auto variable is explicitly
> initialized, compiler will do nothing.
> Looks like for structure variables we need some special handling to
> initialize the paddings.
> Need to study this a little bit and see how to fix it.
Deterministic padding init is a big part of this defense since that's
where the bulk of the really hard-to-find problems have existed (i.e. in
padding holes in structures that got copied around).
> > The last case is for switch variables outside of case statements, like
> > "var" here:
> >
> > switch (path) {
> > unsigned long var;
> >
> > case ..:
> > ...
> > case ..:
> > ...
> > ...
> > }
> >
>
> Will study this case more.
For the kernel, this is a low priority, since I purged the code of all
variable declarations outside of an execution path[1]. For reference,
here's the Clang bug for the same problem[2] (though latest Clang git
appears to no longer exhibit this issue, so they may have fixed it).
-Kees
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?qt=grep&q=Distribute+switch+variables+for+initialization
[2] https://bugs.llvm.org/show_bug.cgi?id=44916
--
Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Feb 25, 2021 at 05:56:38PM -0600, Qing Zhao wrote:
> Just noticed that you didn’t add -fauto-var-init-approach=D to the command
> line.
Ah-ha! I didn't realize that was needed; thanks. However, now some of the
sources crash in a different way. Here's the reproducer:
$ cat poc.i
struct a {
int b;
int array[];
};
void c() {
struct a d;
}
$ gcc -ftrivial-auto-var-init=pattern -fauto-var-init-approach=D -c /dev/null
poc.i
during RTL pass: expand
poc.i: In function ‘c’:
poc.i:6:12: internal compiler error: in build_pattern_cst, at tree.c:2652
6 | struct a d;
|^
0x75b572 build_pattern_cst(tree_node*)
../../../gcc/gcc/tree.c:2652
0x10db116 build_pattern_cst(tree_node*)
../../../gcc/gcc/tree.c:2612
0xb8a230 expand_DEFERRED_INIT
../../../gcc/gcc/internal-fn.c:2980
0x970e17 expand_call_stmt
../../../gcc/gcc/cfgexpand.c:2749
0x970e17 expand_gimple_stmt_1
../../../gcc/gcc/cfgexpand.c:3844
0x970e17 expand_gimple_stmt
../../../gcc/gcc/cfgexpand.c:4008
0x9766b3 expand_gimple_basic_block
../../../gcc/gcc/cfgexpand.c:6045
0x9780d6 execute
../../../gcc/gcc/cfgexpand.c:6729
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See <https://gcc.gnu.org/bugs/> for instructions.
I assume it's not handling the flex-array happily?
--
Kees Cook
Re: [[GCC13][Patch][V3] 1/2] Add a new option -fstrict-flex-array[=n] and new attribute strict_flex_array
On Wed, Aug 31, 2022 at 08:16:49PM +, Qing Zhao wrote: > > > On Aug 31, 2022, at 4:09 PM, Joseph Myers wrote: > > > > On Wed, 31 Aug 2022, Qing Zhao wrote: > > > >>>> When -std=gnu89 + -fstrict-flex-array=3 (ONLY C99 flexible array member > >>>> [] is treated as a valid flexible array) present together, > >>> > >>> That seems reasonable enough without a warning. If people want a warning > >>> for flexible array members in older language modes, they can use > >>> -pedantic; I don't think we need to warn for any particular > >>> -fstrict-flex-array modes there. > >> > >> So, you mean, > >> > >> 1. GCC with -std=gnu89 support all [0], [1], and [] as Flexible array > >> member; > >> 2. Therefore. -std=gnu89 + -fstrict-flex-array=3 does not need a warning; > >> > >> ? > > > > Yes. > > > >> Then, how about: > >> > >> -std=c89: > >> > >> 1. GCC with -std=c89 also support all [0], [1], and [] as Flexible array > >> member; > >> 2, therefore, -std=c89 + -fstrict-flex-array does not need a warning too. > >> > >> ? > > > > Yes. > > > > Okay, I am fine with this. > > Richard and Kees, what’s your opinion on this? Agreed: I think it's fine not to warn about these "conflicting" flags in those cases. It looks like the C standard warnings about flexible arrays are already hidden behind -Wpedantic, so nothing else is needed, IMO. Using -fstrict-flex-arrays just enforces that warning. ;) -- Kees Cook
Re: [[GCC13][Patch][V3] 1/2] Add a new option -fstrict-flex-array[=n] and new attribute strict_flex_array
On Wed, Aug 31, 2022 at 08:35:12PM +, Qing Zhao wrote: > One of the major purposes of the new option -fstrict-flex-array is to > encourage standard conforming programming style. > > So, it might be reasonable to treat -fstrict-flex-array similar as -pedantic > (but only for flexible array members)? > If so, then issuing warnings when the standard doesn’t support is reasonable > and desirable. I guess the point is that "-std=c89 -fstrict-flex-arrays=3" leaves "[]" available for use still? I think this doesn't matter. If someone wants it to be really strict, they'd just add -Wpedantic. -- Kees Cook
Re: [COMMITTED][patch][version 9]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Thu, Sep 09, 2021 at 10:49:11PM +, Qing Zhao wrote: > Hi, FYI > > I just committed the following patch to gcc upstream: > > > https://gcc.gnu.org/pipermail/gcc-cvs/2021-September/353195.html Hurray! Thank you so much for working on this, and thanks also to the reviewers and everyone else poking at it. I will go update my Linux Plumbers slides to say "supported" instead of "proposed". :) -Kees -- Kees Cook
Re: [patch][gcc12-changes] Add a new item about the support for automatic static variable initialization
On Tue, Sep 28, 2021 at 08:31:13PM +, Qing Zhao wrote: > Hi, > > This is the patch for the gcc12 changes per your request. > > Kees provided most of the wording. > > Please take a look and let’s know whether it’s good for commit? > > thanks. > > Qing > > > > > From: qing zhao > Date: Tue, 28 Sep 2021 12:01:42 -0700 > Subject: [PATCH] gcc-12/changes.html: Uninitialized stack variables > initialization update > > * htdocs/gcc-12/changes.html (Eliminating uninitialized variables): > Item about the support for automatic static variable initialization. > --- > htdocs/gcc-12/changes.html | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/htdocs/gcc-12/changes.html b/htdocs/gcc-12/changes.html > index 1f156a9..8e2979c 100644 > --- a/htdocs/gcc-12/changes.html > +++ b/htdocs/gcc-12/changes.html > @@ -245,6 +245,25 @@ a work-in-progress. > > Other significant improvements > > +Eliminating uninitialized variables > + > + > + GCC can now initialize all stack variables implicitly, including > + padding. This is intended to eliminate all classes of uninitialized > + stack variable flaws. Lack of explicit initialization will still > + warn when -Wuninitialized is active. For best > + debugging, use of the new command-line option > + -ftrivial-auto-var-init=pattern can be used to fill > + variables with a repeated 0xFE pattern, which tends to illuminate > + many bugs (e.g. pointers receive invalid addresses, sizes and indices > + are very large). For best production results, the new command-line > + option -ftrivial-auto-var-init=zero can be used to > + fill variables with 0x00, which tends to provide a safer state for > + bugs (e.g. pointers are NULL, strings are NULL filled, and sizes Minor nit: I've always been corrected that "NULL" refers to a pointer, and "NUL" refers to the "null character", so the latter use of NULL should be "NUL": ... pointers are NULL, strings are NUL filled, and size ... I mix this up all the time, so apologies if that got introduced by me! :) -Kees > + and indices are 0). > + > + > + > Debugging formats > > > -- > 1.9.1 > > -- Kees Cook
Re: [patch][gcc12-changes] Add a new item about the support for automatic static variable initialization
On Wed, Sep 29, 2021 at 02:43:35PM +, Qing Zhao wrote: > FYI, just committed the change: > > https://gcc.gnu.org/gcc-12/changes.html Looks great to me; thanks! :) -Kees -- Kees Cook
Re: [RFC][patch for gcc12][version 1] add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Tue, Jun 01, 2021 at 04:35:53PM -0400, David Malcolm wrote: > [...] > Did this patch get reviewed/approved? It's still under review, but I think it's close. > Is the latest version still this one: > https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565581.html > or is there a more recent version that should be reviewed? Yup, here's the latest (v3): https://gcc.gnu.org/pipermail/gcc-patches/2021-May/570208.html > (I don't think I'm qualified to approve the patch, I'm just a fan of > the approach. FWIW I've been experimenting with extending -fanalyzer > to detect infoleaks in the kernel, whereas AIUI this patch is about > mitigating them) Thanks for your interest! If you patch your GCC with this, it should Just Work in the kernel (i.e. you can set CONFIG_INIT_STACK_ALL_ZERO=y) > Hope this is constructive Yup! Please report back any testing; that'll help show people are interested in the feature. :) -- Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Mon, Jun 07, 2021 at 09:48:41AM +0200, Richard Biener wrote:
> On Thu, 27 May 2021, Qing Zhao wrote:
> > @@ -5001,6 +5185,17 @@ gimplify_init_constructor (tree *expr_p, gimple_seq
> > *pre_p, gimple_seq *post_p,
> > /* If a single access to the target must be ensured and all
> > elements
> > are zero, then it's optimal to clear whatever their number.
> > */
> > cleared = true;
> > + else if (flag_trivial_auto_var_init > AUTO_INIT_UNINITIALIZED
> > +&& !TREE_STATIC (object)
> > +&& type_has_padding (type))
> > + /* If the user requests to initialize automatic variables with
> > +paddings inside the type, we should initialize the paddings
> > too.
> > +C guarantees that brace-init with fewer initializers than
> > members
> > +aggregate will initialize the rest of the aggregate as-if it
> > were
> > +static initialization. In turn static initialization
> > guarantees
> > +that pad is initialized to zero bits.
> > +So, it's better to clear the whole record under such
> > situation. */
> > + cleared = true;
> >
> > so here we have padding as well - I think this warrants to be controlled
> > by an extra option? And we can maybe split this out to a separate
> > patch? (the whole padding stuff)
> >
> > Clang does the padding initialization with this option, shall we be
> > consistent with Clang?
>
> Just for the sake of consistency? No. Is there a technical reason
> for this complication? Say we have
>
> struct { short s; int i; } a;
>
> what's the technical reason to initialize the padding? I might
> be tempted to use -ftrivial-auto-init but I'd definitely don't
> want to spend cycles/instructions initializing the padding in the
> above struct.
Yes, this is very important. This is one of the more common ways memory
content leaks happen in programs (especially the kernel). e.g.:
struct example {
short s;
int i;
};
struct example instance = { .i = foo };
While "s" gets zeroed, the padding may not, and may contain prior memory
contents. Having this be deterministically zero is important for this
feature. If the structure gets byte-copied to a buffer (e.g. syscall,
etc), the padding will go along for the ride.
--
Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Mon, Jun 07, 2021 at 04:18:46PM +, Qing Zhao wrote: > Hi, > > > On Jun 7, 2021, at 2:53 AM, Richard Biener wrote: > > > >> > >> To address the above suggestion: > >> > >> My study shows: the call to __builtin_clear_padding is expanded during > >> gimplification phase. > >> And there is no __bultin_clear_padding expanding during rtx expanding > >> phase. > >> However, for -ftrivial-auto-var-init, padding initialization should be > >> done both in gimplification phase and rtx expanding phase. > >> since the __builtin_clear_padding might not be good for rtx expanding, > >> reusing __builtin_clear_padding might not work. > >> > >> Let me know if you have any more comments on this. > > > > Yes, I didn't suggest to literally emit calls to __builtin_clear_padding > > but instead to leverage the lowering code, more specifically share the > > code that figures _what_ is to be initialized (where the padding is) > > and eventually the actual code generation pieces. That might need some > > refactoring but the code where padding resides should be present only > > a single time (since it's quite complex). > > Okay, I see your point here. > > > > > Which is also why I suggested to split out the padding initialization > > bits to a separate patch (and option). > > Personally, I am okay with splitting padding initialization from this current > patch, > Kees, what’s your opinion on this? i.e, the current -ftrivial-auto-var-init > will NOT initialize padding, we will add another option to > Explicitly initialize padding. If two new options are needed, that's fine. But "-ftrivial-auto-var-init" needs to do both (it is _not_ getting fully initialized if padding isn't included). And would be a behavioral mismatch between Clang and GCC. :) -- Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Tue, Jun 08, 2021 at 09:37:30AM +0200, Richard Biener wrote: > On Mon, 7 Jun 2021, Qing Zhao wrote: > > On Jun 7, 2021, at 2:48 AM, Richard Biener > > mailto:rguent...@suse.de>> wrote: > > > > Meh - can you try using a mailer that does proper quoting? It's difficult > > to spot your added comments. Will try anyway (and sorry for the delay) > > > > Only the email replied to gcc-patch alias had this issue, all the other > > emails I sent are fine. Not sure why? > > All your mails have this problem for me, it makes it quite difficult to > follow the conversation. I think the first step is to make sure the MUA is sending "text only" emails. Then configure the "quoting style" to do the standard "> "-style. What email client are you using? -- Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Tue, Jun 08, 2021 at 09:41:38AM +0200, Richard Biener wrote:
> On Mon, 7 Jun 2021, Qing Zhao wrote:
>
> > Hi,
> >
> > > On Jun 7, 2021, at 2:53 AM, Richard Biener wrote:
> > >
> > >>
> > >> To address the above suggestion:
> > >>
> > >> My study shows: the call to __builtin_clear_padding is expanded during
> > >> gimplification phase.
> > >> And there is no __bultin_clear_padding expanding during rtx expanding
> > >> phase.
> > >> However, for -ftrivial-auto-var-init, padding initialization should be
> > >> done both in gimplification phase and rtx expanding phase.
> > >> since the __builtin_clear_padding might not be good for rtx expanding,
> > >> reusing __builtin_clear_padding might not work.
> > >>
> > >> Let me know if you have any more comments on this.
> > >
> > > Yes, I didn't suggest to literally emit calls to __builtin_clear_padding
> > > but instead to leverage the lowering code, more specifically share the
> > > code that figures _what_ is to be initialized (where the padding is)
> > > and eventually the actual code generation pieces. That might need some
> > > refactoring but the code where padding resides should be present only
> > > a single time (since it's quite complex).
> >
> > Okay, I see your point here.
> >
> > >
> > > Which is also why I suggested to split out the padding initialization
> > > bits to a separate patch (and option).
> >
> > Personally, I am okay with splitting padding initialization from this
> > current patch,
> > Kees, what’s your opinion on this? i.e, the current -ftrivial-auto-var-init
> > will NOT initialize padding, we will add another option to
> > Explicitly initialize padding.
>
> It would also be possible to have -fauto-var-init, -fauto-var-init-padding
> and have -ftrivial-auto-var-init for clang compatibility enabling both.
Sounds good to me!
> Or -fauto-var-init={zero,pattern,padding} and allow
> -fauto-var-init=pattern,padding to be specified. Note there's also
> padding between auto variables on the stack - that "trailing"
> padding isn't initialized either? (yes, GCC sorts variables to minimize
> that padding) For example for
>
> void foo()
> {
> char a[3];
> bar (a);
> }
>
> there's 12 bytes padding after 'a', shouldn't we initialize that? If not,
> why's other padding important to be initialized?
This isn't a situation that I'm aware of causing real-world problems.
The issues have all come from padding within an addressable object. I
haven't tested Clang's behavior on this (and I have no kernel tests for
this padding), but I do check for trailing padding, like:
struct test_trailing_hole {
char *one;
char *two;
char *three;
char four;
/* "sizeof(unsigned long) - 1" byte padding hole here. */
};
-Kees
--
Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Tue, Jun 08, 2021 at 05:32:32PM +, Qing Zhao wrote: > Thanks a lot. > > Kees. > > Do you have the same issue with my emails? Some of them, yes. This one was fine. > I see this problem with my email mostly to part of the emails that were sent > to gcc-patches alias. > Other emails are fine. > > > On Jun 8, 2021, at 11:56 AM, Kees Cook wrote: > > > > On Tue, Jun 08, 2021 at 09:37:30AM +0200, Richard Biener wrote: > >> On Mon, 7 Jun 2021, Qing Zhao wrote: > >>> On Jun 7, 2021, at 2:48 AM, Richard Biener > >>> mailto:rguent...@suse.de>> wrote: > >>> > >>> Meh - can you try using a mailer that does proper quoting? It's difficult > >>> to spot your added comments. Will try anyway (and sorry for the delay) > >>> > >>> Only the email replied to gcc-patch alias had this issue, all the other > >>> emails I sent are fine. Not sure why? > >> > >> All your mails have this problem for me, it makes it quite difficult to > >> follow the conversation. > > > > I think the first step is to make sure the MUA is sending "text only" > > emails. Then configure the "quoting style" to do the standard "> "-style. > > > > What email client are you using? > > I am using Mac’s Apple Mail client on my computer. > > I have been using this mail client for a long time, but only had such issues > recently. I share your pain! Gmail frequently likes making tiny breaking changes too. :) > Really not sure what’s going on. > > I will try to figure this out. Thanks! -- Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Fri, Jun 11, 2021 at 03:49:02PM +, Qing Zhao wrote: > > On Jun 11, 2021, at 6:12 AM, Richard Biener wrote: > > [...] > > > > The main point is that you need to avoid building the explicit initializer > > only to have it consumed by assignment expansion. If you want to keep > > all the singing and dancing (as opposed to maybe initializing with a > > 0x1 byte pattern) then I think for efficiency you still want to > > block-initialize the variable and then only fixup the special fields. > > Yes, this is a good idea. > > We can memset the whole structure with repeated pattern “0xAA” first, > Then mixup BOOLEAN_TYPE and POINTER TYPE for 32-bit platform. > That might be more efficient. > > > > > But as said, all this is quite over-designed IMHO and simply > > zeroing everything would be much simpler and good enough. > > So, the fundenmental questions are: > > 1. do we need the functionality of “Pattern Initialization” for debugging > purpose? > I see that other compilers support both Zero initialization and Pattern > initialization. (Clang and Microsoft compiler) > > http://lists.llvm.org/pipermail/cfe-dev/2020-April/065221.html > https://msrc-blog.microsoft.com/2020/05/13/solving-uninitialized-stack-memory-on-windows/ > Pattern init is used in development build for debugging purpose, zero init is > used in production build for security purpose. Correct -- "pattern" is much better about triggering all kinds of problems, and suitable for debug builds. "zero" is less disruptive and generally provides a safer failure mode for production builds. > So, I assume that GCC might want to provide similar functionality? But I am > open on this. > > Kees, will Kernel use “Pattern initialization” feature? It is currently support, yes. Note that if I had to choose between "nothing" and "only zero", I will happily take "only zero". However, it seems like pattern init isn't much more of an addition in this series. > 2. Since “Pattern initialization” is just used for debugging purpose, the > runtime and code size overhead might not be that > Important at all, right? That has been my impression, yes. Thanks! -Kees -- Kees Cook
Re: [PATCH][version 3]add -ftrivial-auto-var-init and variable attribute "uninitialized" to gcc
On Fri, Jun 11, 2021 at 01:04:09PM +0200, Richard Biener wrote:
> On Tue, 8 Jun 2021, Kees Cook wrote:
>
> > On Tue, Jun 08, 2021 at 09:41:38AM +0200, Richard Biener wrote:
> > > On Mon, 7 Jun 2021, Qing Zhao wrote:
> > >
> > > > Hi,
> > > >
> > > > > On Jun 7, 2021, at 2:53 AM, Richard Biener wrote:
> > > > >
> > > > >>
> > > > >> To address the above suggestion:
> > > > >>
> > > > >> My study shows: the call to __builtin_clear_padding is expanded
> > > > >> during gimplification phase.
> > > > >> And there is no __bultin_clear_padding expanding during rtx
> > > > >> expanding phase.
> > > > >> However, for -ftrivial-auto-var-init, padding initialization should
> > > > >> be done both in gimplification phase and rtx expanding phase.
> > > > >> since the __builtin_clear_padding might not be good for rtx
> > > > >> expanding, reusing __builtin_clear_padding might not work.
> > > > >>
> > > > >> Let me know if you have any more comments on this.
> > > > >
> > > > > Yes, I didn't suggest to literally emit calls to
> > > > > __builtin_clear_padding
> > > > > but instead to leverage the lowering code, more specifically share the
> > > > > code that figures _what_ is to be initialized (where the padding is)
> > > > > and eventually the actual code generation pieces. That might need
> > > > > some
> > > > > refactoring but the code where padding resides should be present only
> > > > > a single time (since it's quite complex).
> > > >
> > > > Okay, I see your point here.
> > > >
> > > > >
> > > > > Which is also why I suggested to split out the padding initialization
> > > > > bits to a separate patch (and option).
> > > >
> > > > Personally, I am okay with splitting padding initialization from this
> > > > current patch,
> > > > Kees, what’s your opinion on this? i.e, the current
> > > > -ftrivial-auto-var-init will NOT initialize padding, we will add
> > > > another option to
> > > > Explicitly initialize padding.
> > >
> > > It would also be possible to have -fauto-var-init, -fauto-var-init-padding
> > > and have -ftrivial-auto-var-init for clang compatibility enabling both.
> >
> > Sounds good to me!
> >
> > > Or -fauto-var-init={zero,pattern,padding} and allow
> > > -fauto-var-init=pattern,padding to be specified. Note there's also
> > > padding between auto variables on the stack - that "trailing"
> > > padding isn't initialized either? (yes, GCC sorts variables to minimize
> > > that padding) For example for
> > >
> > > void foo()
> > > {
> > > char a[3];
> > > bar (a);
> > > }
> > >
> > > there's 12 bytes padding after 'a', shouldn't we initialize that? If not,
> > > why's other padding important to be initialized?
> >
> > This isn't a situation that I'm aware of causing real-world problems.
> > The issues have all come from padding within an addressable object. I
> > haven't tested Clang's behavior on this (and I have no kernel tests for
> > this padding), but I do check for trailing padding, like:
> >
> > struct test_trailing_hole {
> > char *one;
> > char *two;
> > char *three;
> > char four;
> > /* "sizeof(unsigned long) - 1" byte padding hole here. */
> > };
>
> Any justification why tail padding for
>
> struct foo { double x; char x[3]; } a;
>
> is important but not for
>
> char x[3];
>
> ? It does look like an odd inconsistency to me.
The problem is with sizeof() and the various compounding results related
to it. Namely, things that do whole-struct copies (which is unfortunately
common in the kernel) will include the padding for "a" since it is within
the object, as represented by sizeof(), but not for x:
#include
int main(void)
{
struct foo { double y; char x[3]; } a;
char x[3];
printf("a: %zu (a.y: %zu, a.x: %zu)\n", sizeof(a), sizeof(a.y),
sizeof(a.x));
printf("x: %zu\n", sizeof(x));
return 0;
}
a: 16 (a.y: 8, a.x: 3)
x: 3
And it gets worse with structs-within-structs, etc.
--
Kees Cook
Re: [RFC PATCH 0/1] implement TLS register based stack canary for ARM
On Thu, Oct 21, 2021 at 06:34:04PM +0200, Ard Biesheuvel wrote: > On Thu, 21 Oct 2021 at 12:23, Ard Biesheuvel wrote: > > > > Bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102352 > > > > In the Linux kernel, user processes calling into the kernel are > > essentially threads running in the same address space, of a program that > > never terminates. This means that using a global variable for the stack > > protector canary value is problematic on SMP systems, as we can never > > change it unless we reboot the system. (Processes that sleep for any > > reason will do so on a call into the kernel, which means that there will > > always be live kernel stack frames carrying copies of the canary taken > > when the function was entered) > > > > AArch64 implements -mstack-protector-guard=sysreg for this purpose, as > > this permits the kernel to use different memory addresses for the stack > > canary for each CPU, and context switch the chosen system register with > > the rest of the process, allowing each process to use its own unique > > value for the stack canary. > > > > This patch implements something similar, but for the 32-bit ARM kernel, > > which will start using the user space TLS register TPIDRURO to index > > per-process metadata while running in the kernel. This means we can just > > add an offset to TPIDRURO to obtain the address from which to load the > > canary value. > > > > The patch is a bit rough around the edges, but produces the correct > > results as far as I can tell. > > This is a lie LOL. > > > However, I couldn't quite figure out how > > to modify the patterns so that the offset will be moved into the > > immediate offset field of the LDR instructions, so currently, the ADD of > > the offset is always a distinct instruction. > > > > ... and this is no longer true now that I fixed the correctness > problem. I will be sending out a v2 shortly, so please disregard this > one for now. Heh, I hadn't even had a chance to test it, so I'll hold off. :) Thanks! -Kees -- Kees Cook
Re: [PATCH v3 1/1] [ARM] Add support for TLS register based stack protector canary access
On Tue, Oct 26, 2021 at 10:18:36AM +0200, Ard Biesheuvel wrote: > Add support for accessing the stack canary value via the TLS register, > so that multiple threads running in the same address space can use > distinct canary values. This is intended for the Linux kernel running in > SMP mode, where processes entering the kernel are essentially threads > running the same program concurrently: using a global variable for the > canary in that context is problematic because it can never be rotated, > and so the OS is forced to use the same value as long as it remains up. > > Using the TLS register to index the stack canary helps with this, as it > allows each CPU to context switch the TLS register along with the rest > of the process, permitting each process to use its own value for the > stack canary. > > 2021-10-21 Ard Biesheuvel > > * config/arm/arm-opts.h (enum stack_protector_guard): New > * config/arm/arm-protos.h (arm_stack_protect_tls_canary_mem): > New > * config/arm/arm.c (TARGET_STACK_PROTECT_GUARD): Define > (arm_option_override_internal): Handle and put in error checks > for stack protector guard options. > (arm_option_reconfigure_globals): Likewise > (arm_stack_protect_tls_canary_mem): New > (arm_stack_protect_guard): New > * config/arm/arm.md (stack_protect_set): New > (stack_protect_set_tls): Likewise > (stack_protect_test): Likewise > (stack_protect_test_tls): Likewise > * config/arm/arm.opt (-mstack-protector-guard): New > (-mstack-protector-guard-offset): New. > > Signed-off-by: Ard Biesheuvel I can't speak to the specific implementation details here, but this builds for me, and behaves as expected. I get a working kernel[1], and have verified[2] that we have per-task canaries for arm32. :) Yay! Tested-by: Kees Cook Who's best to review and commit this? Qing, is something you're able to review? Thanks! -Kees [1] https://lore.kernel.org/linux-arm-kernel/20211021142516.1843042-1-a...@kernel.org/ [2] https://lore.kernel.org/linux-hardening/2021103826.330653-3-keesc...@chromium.org/ -- Kees Cook
