On Fri, Apr 25, 2025 at 04:56:52PM +0000, Qing Zhao wrote: > > > > On Apr 24, 2025, at 13:07, Kees Cook <k...@kernel.org> wrote: > > > > On Thu, Apr 24, 2025 at 04:36:14PM +0000, Qing Zhao wrote: > >> > >>> On Apr 24, 2025, at 11:59, Martin Uecker <uec...@tugraz.at> wrote: > >>> > >>> Am Donnerstag, dem 24.04.2025 um 15:15 +0000 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: > > > > <source>: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 <stdlib.h> > > 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