On Fri, Jul 12, 2024 at 10:55:16AM -0700, Kees Cook wrote: > > What will actually break if you 'fix' this? Given that inlining (see > > below) changes the rules willy nilly, I feel we can (and should!) just > > fix this. > > I'm not sure -- I have kind of given up on "standard" C helping with any > of this. I look to consistent language extensions now, and where there > isn't any, we've been trying to create them. :P
Yeah, arguing a committee is mostly a waste of time, also, they typically listen a lot more when you say, here these two compilers have implemented it and this Linux thing uses it. So yeah, language extensions are it. > And we're not alone: > Apple's -fbounds-safety stuff[1] looks good too, and overlaps with what > we were already designing with the "counted_by" attribute: > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/ > (We borrowed the "counted_by" name, which is better than what we were > calling it: "element_count".) Yep, I read that a while back. I think you referenced it in one of them other threads where we disagreed over struct_size() :-) > > > Does report the expected things for _bdos internally (48), but not for > > > sizeof (8) nor _bos (SIZE_MAX). Of course if we inline it, _bos starts > > > working and, along with _bdos, realizes it was lied to, and reports > > > 32 again. > > > > WTF ?!?! How can all this be so inconsistent and why are people okay > > with that? > > This. A thousands times, this. I'm really not okay with it, and we've > been working to get rid of every ambiguity we trip over. It's made sane > bounds checking in Linux extremely hard to get right. Yeah, not just Linux I imagine. The rules are so insane it's near useless. I'd say press onwards with the language extension, it's not like Linux kernel is written in ANSI/ISO C anyway :-) > For more fun with array bounds, the one that absolutely floored me was > the exception over trailing arrays: > > struct middle_t { > u8 array[6]; > int foo; > } *middle; > > __builtin_object_size(middle->array, 1) == 6 > > struct trailing_t { > int foo; > u8 array[6]; > } *trailing; > > __builtin_object_size(trailing->array, 1) == SIZE_MAX ("unknown") WTF :-) > > So I'm not entirely sure I agree with that argument. Yes, ®s->bx is > > 'unsigned long *' and sizeof(unsigned long) is 8 (if we assume 64bit). > > However, you can also read it as the point of pt_regs where bx sits -- > > which is a far more sensible interpretation IMO. > > > > Because then we're looking at struct pt_regs and an offset therein. > > Right -- the way to make this unambiguous has been to make sure there > is an addressable object which contains the elements in question. For > the least disruption, the best we were able to do is introduce the > struct_group() helper. It's internally ugly, but it works. That macro is fairly trivial, nowhere near as ugly as struct_size() :-) But urgh... can't we do something like: void *memcpy_off(void *dst, const void *src, size_t off, size_t n) { memcpu(dst, src+off, n); return dst; } And then you can write: memcpy_off(args, regs, offsetof(*regs, bx), 6); I mean, that sucks, but possilby less than struct_group() does. [ also, we should probably do: #defime offsetof(t, m) __builtin_offsetof(typeof(t), m) ] > > So really pt_regs *is* an array of unsigned long, and I feel it is > > really unfortunate we cannot express this in a way that is more concise. > > A way to do this would be: > > struct pt_regs { > union { > struct { > unsigned long bx; > unsigned long cx; > unsigned long dx; > unsigned long si; > unsigned long di; > unsigned long bp; > }; > unsigned long syscall_regs[6]; > }; > unsigned long ax; > ... > }; > > Now regs->syscall_regs is addressable, sized, etc. "bx" means just bx, > and "syscall_regs" means all 6. In this case I would just make all of pt_regs a union with one giant array (much like some archs already have IIRC). > I wrote up a bunch of notes about much of this horror last year here: > https://people.kernel.org/kees/bounded-flexible-arrays-in-c Oh, yeah, I think I saw that fly by on hackernews a while ago.
