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
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
cross fingers*
-Kees
--
Kees Cook
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
actly 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
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
_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
> 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
t 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
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
rhaps, 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
rg/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
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)
> >
ide 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.
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
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
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
gt; 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
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
ion 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
l:2 xpass:0 skip:0 error:0
Thanks!
-Kees
--
Kees Cook
lds the Linux kernel where we have almost 300
instances of "counted_by" attributes.
Yay!
-Kees
--
Kees Cook
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,
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
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.
> >>
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
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
hould 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
rom 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
ble() && 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
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,
>
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
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
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 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:
&
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
> &
dexes? I'd find that kind of awkward for the kernel... but I feel like
I've misunderstood something. :)
-Kees
--
Kees Cook
, 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
is be a
separate warning the kernel can turn off initially?
-Kees
--
Kees Cook
tribute__(optimize ("wrapv"))
>
> To mark the specific function to be wrapped around.
>
> However, this attribute does not work for linux kernel due to the following
> reason:
>
> Attribute optimize should be only used for debugging purpose;
> The kernel has banned its usage;
>
> So, a new attribute was requested from Linux kernel security:
>
> request wrap-around behavior for specific function (PR102317)
> __attribute__((wrapv))
>
> Is this request reasonable?
After working through this discussion, I'd say it's likely more helpful
to have a way to disable the sanitizers for a given function (or
variable). i.e. The goal for the kernel would that untrapped wrap-around
would be the very rare exception. e.g. our refcount_t implementation:
https://elixir.bootlin.com/linux/v6.5/source/include/linux/refcount.h#L200
Then we can continue to build the kernel with -fno-strict-overflow (to
avoid UB), but gain sanitizer coverage for all run-time wraps, except
for the very few places where we depend on it. Getting there will also
take some non-trivial refactoring on our end, but without the sanitizers
we're unlikely to find them all.
--
Kees Cook
doing things like "if (ulong + offset < ulong) { ... }":
https://elixir.bootlin.com/linux/v6.5/source/drivers/crypto/axis/artpec6_crypto.c#L1187
This is easy for a static analyzer to find and we can replace it with a
non-wrapping test (e.g. __builtin_add_overflow()), but we'll not find
them all immediately, especially for the signed and pointer cases.
So, I need to retain the "everything wraps" behavior while still being
able to detect when it happens.
--
Kees Cook
id=80998
Ah, thanks for the link! And checking it just now it seems like Clang's
implementation doesn't work. Fun times.
-Kees
--
Kees Cook
ee with 201 annotations[1] added. Things work as expected. :)
-Kees
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=devel/next-20230825/counted_by
--
Kees Cook
stall/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
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
>
c/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
; is used uninitialized
[-Wuninitialized]
366 | p->array[0] = 0;
| ^~~
Yay! :)
-Kees
--
Kees Cook
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
nel'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
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
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
> >
zeof(*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
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
ram 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
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 tha
n 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
n,
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
nt_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
"counted_by":
https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/
Thanks again!
-Kees
--
Kees Cook
he Linux kernel, and this
is behaving as I'd expect. This makes my life MUCH easier -- many fewer
false positives for our bounds checking. :)
-Kees
--
Kees Cook
27;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
hen such structure is not at the end.
>
> Let me know if you have any comment or suggestions.
FWIW this all sounds correct to me.
--
Kees Cook
zed-type-not-at-end]
> > struct Y { struct X x; int end; };
> >
> > ^
>
> Clang’s warning message is clearer.
> >
> > looking at PR77650 what seems missing there is the semantics of this
> > extension as expected/required by the glibc use. comment#5 seems
> > to suggest that for my example above its expected that
> > Y.x.data[0] aliases Y.end?!
>
> Should we mentioned this alias relationship in the doc?
>
> > There must be a better way to write
> > the glibc code and IMHO it would be best to deprecate this extension.
>
> Agreed. This is really a bad practice, should be deprecated.
> We can give warning first in this release, and then deprecate this extension
> in a latter release.
Right -- this can lead (at least) to type confusion and other problems
too. We've been trying to remove all of these overlaps in the Linux
kernel. I mention it the "Overlapping composite structure members"
section at https://people.kernel.org/kees/bounded-flexible-arrays-in-c
--
Kees Cook
suse. I think it’s necessary
> To provide a seperate warning to only issue flexible array misuse.
>
> Let me know if you have any more comments on this.
Okay, that seems good. Given that -Warray-bounds is part of -Wall, what
should happen for -Wstrict-flex-arrays=N?
--
Kees Cook
On Thu, Dec 01, 2022 at 05:04:02PM +, Qing Zhao wrote:
>
>
> > On Dec 1, 2022, at 11:42 AM, Kees Cook wrote:
> >
> > On Wed, Nov 30, 2022 at 02:25:56PM +, Qing Zhao wrote:
> >> '-Wstrict-flex-arrays'
> >> Warn about in
renced as a flexible array member.
>
> At the same time, -Warray-bounds is updated:
Why is there both -Wstrict-flex-arrays and -Warray-bounds? I thought
only the latter was going to exist?
Are you trying to split code gen (-fstrict-flex-arrays) from warnings?
Is that needed?
--
Kees Cook
Nov 2022, Qing Zhao wrote:
> >>
> >>>
> >>>
> >>>> On Nov 18, 2022, at 11:31 AM, Kees Cook wrote:
> >>>>
> >>>> On Fri, Nov 18, 2022 at 03:19:07PM +, Qing Zhao wrote:
> >>>>> Hi, Richard,
>
-Warray-bounds. That
it is in -Wall is _good_ for this reason. :) No one is going to add
-fstrict-flex-arrays (at any level) without understanding what it does
and wanting those effects on -Warray-bounds.
--
Kees Cook
wants
it to be really strict, they'd just add -Wpedantic.
--
Kees Cook
t; >> 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
,
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
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
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
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.ker
odify 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
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
nter, 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
ewers and everyone else poking at it.
I will go update my Linux Plumbers slides to say "supported" instead of
"proposed". :)
-Kees
--
Kees Cook
without any issue.
>
> Please take a look and let me know any issue.
All my kernel tests pass; this looks great! Thank you. :)
--
Kees Cook
e
8 | struct weird obj = { .val = val };
| ^~~
--
Kees Cook
On Wed, Jul 14, 2021 at 07:30:45PM +, Qing Zhao wrote:
> Hi, Kees,
>
>
> > On Jul 14, 2021, at 2:11 PM, Kees Cook wrote:
> >
> > On Wed, Jul 14, 2021 at 02:09:50PM +, Qing Zhao wrote:
> >> Hi, Richard,
> >>
> >>>
else
> cleared = false;
>
> Then the paddings are also initialized to zeroes with this option. (Even for
> -ftrivial-auto-var-init=pattern).
Thanks! I've tested with the attached patch to v4 and it passes all my
tests again.
> Is the above change Okay? (With this
really, any program). Without this, things are still left
uninitialized in the padding of structs.
A separate patch is fine by me; my only desire is to still have it be
part of -ftrivial-auto-var-init when it's all done. :)
Thanks!
--
Kees Cook
On Tue, Jul 13, 2021 at 02:29:33PM -0700, Kees Cook wrote:
> I've extracted the kernel test to build for userspace, and it behaves
> the same way. See attached "stackinit.c".
I've adjusted this slightly (the "static" tests weren't testing the
correct
On Mon, Jul 12, 2021 at 08:28:55PM +, Qing Zhao wrote:
> > On Jul 12, 2021, at 12:56 PM, Kees Cook wrote:
> > On Wed, Jul 07, 2021 at 05:38:02PM +, Qing Zhao wrote:
> >> This is the 4th version of the patch for the new security feature for GCC.
> >
[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-February/565840.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2021-April/567754.html
--
Kees Cook
On Tue, Jun 22, 2021 at 09:25:57AM +0100, Richard Sandiford wrote:
> Kees Cook writes:
> > On Mon, Jun 21, 2021 at 03:39:45PM +, Qing Zhao wrote:
> >> So, if “pattern value” is “0x”, then it’s a valid
> >> canonical virtual memory add
F” good for pointer? Or
> “0x” better?
I think 0xFF repeating is fine for this version. Everything else is a
"nice to have" for the pattern-init, IMO. :)
--
Kees Cook
all we
> choose one single repeatable pattern for all the types as you suggested,
> Or chose different patterns for different types as Clang and Microsoft
> compiler’s behavior?
>
> Kees, do you have any comment on this?
>
> How did Linux Kernel use -ftrivial-auto-var-init=pattern feature of CLANG?
It's just used as-is from the compiler, and recommended for "debug
builds".
--
Kees Cook
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,
> > > >
tern 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
.
> 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
>
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
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
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
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
Yup! Please report back any testing; that'll help show people are
interested in the feature. :)
--
Kees Cook
ls_r, NULL, NULL);
> > }
> > + /* When there is no explicit initializer, if the user requested,
> > +We should insert an artifical initializer for this automatic
> > +variable for non vla variables. */
>
> I think we should explain why we can skip VLAs here.
FWIW, in testing, VLAs do get initialized, so I guess there's a separate
place where it happens.
Thanks for the review!
--
Kees Cook
.
> * gcc.target/i386/auto-init-13.c: New test.
> * gcc.target/i386/auto-init-14.c: New test.
> * gcc.target/i386/auto-init-15.c: New test.
> * gcc.target/i386/auto-init-16.c: New test.
> * gcc.target/i386/auto-init-17.c: New test.
> * gcc.target/i386/auto-init-18.c: New test.
> * gcc.target/i386/auto-init-19.c: New test.
> * gcc.target/i386/auto-init-2.c: New test.
> * gcc.target/i386/auto-init-20.c: New test.
> * gcc.target/i386/auto-init-3.c: New test.
> * gcc.target/i386/auto-init-4.c: New test.
> * gcc.target/i386/auto-init-5.c: New test.
> * gcc.target/i386/auto-init-6.c: New test.
> * gcc.target/i386/auto-init-7.c: New test.
> * gcc.target/i386/auto-init-8.c: New test.
> * gcc.target/i386/auto-init-9.c: New test.
>
> **The complete patch is attached as following:
>
>
>
--
Kees Cook
On Fri, Mar 12, 2021 at 03:35:28PM -0600, Qing Zhao wrote:
> Hi, Kees,
>
> I am looking at the structure padding initialization issue. And also have
> some questions:
>
>
> > On Feb 24, 2021, at 10:41 PM, Kees Cook wrote:
> >
> > It looks like there is st
it correctly, I have the following question:
>
>
> > On Feb 26, 2021, at 3:42 PM, Kees Cook wrote:
> >
> > 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
> >&
1 - 100 of 114 matches
Mail list logo
