> On Apr 25, 2025, at 13:18, Kees Cook <k...@kernel.org> wrote: > > 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.
This works. (Is this the only valid usage for this?) So, you want the bound sanitizer to catch the out-of-bound access for the following? return &array_annotated->c[annotated_index + 1]; Qing > > 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