> 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


Reply via email to