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

Reply via email to