Re: [V1][PATCH 0/3] New attribute "element_count" to annotate bounds for C99 FAM(PR108896)

[Qing Zhao <qing.zhao@oracle.com>]

The following is the updated documentation on this new attribute, please let me know any suggestion and comment:

======

'counted_by (COUNT)'
     The 'counted_by' attribute may be attached to the flexible array
     member of a structure.  It indicates that the number of the
     elements of the array is given by the field named "COUNT" in the
     same structure as the flexible array member.  GCC uses this
     information to improve the results of
     '__builtin_dynamic_object_size' and array bound sanitizer.

     For instance, the following declaration:

          struct P {
            size_t count;
            int array[] __attribute__ ((counted_by (count)));
          } *p;

     specifies that the 'array' is a flexible array member whose number
     of elements is given by the field 'count' in the same structure.

     The field that represents the number of the elements should have an
     integer type.  An explicit 'counted_by' annotation defines a
     relationship between two objects, 'p->array' and 'p->count', that
     'p->array' has 'p->count' number of elements available.  This
     relationship must hold even after any of these related objects are
     updated.  It's the user's responsibility to make sure this
     relationship to be kept all the time.  Otherwise the results of the
     '__builtin_dynamic_object_size' and array bound sanitizer might be
     incorrect.

     For instance, the following 2nd update to the field 'count' of the
     above structure will permit out-of-bounds access to the array
     'sbuf>array':

          struct P *sbuf;
          void alloc_buf (size_t nelems)
          {
            sbuf = (int *) malloc (sizeof (struct P) + sizeof (int) * nelems);
            sbuf->count = nelems;
          }
          void use_buf (int index)
          {
            sbuf->count++;
            /* Now the value of sbuf->count and the number
               of elements of sbuf->array is out of sync.  */
            sbuf->array[index] = 0;
            /* then the out-of-bound access to this array
               might not be detected.  */
          }

     The users can use the warning option '-Wcounted-by-attribute' to
     detect such user errors during compilation time, or the sanitizer
     option '-fsanitize=counted-by-attribute' to detect such user errors
     during runtime.

=====

Qing

> On Jul 7, 2023, at 11:47 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> 
> 
>> On Jul 6, 2023, at 5:10 PM, Martin Uecker <uecker@tugraz.at> wrote:
>> 
>> Am Donnerstag, dem 06.07.2023 um 18:56 +0000 schrieb Qing Zhao:
>>> Hi, Kees,
>>> 
>>> I have updated my V1 patch with the following changes:
>>> A. changed the name to "counted_by"
>>> B. changed the argument from a string to an identifier
>>> C. updated the documentation and testing cases accordingly.
>>> 
>>> And then used this new gcc to test https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c (with the following change)
>>> [opc@qinzhao-ol8u3-x86 Kees]$ !1091
>>> diff array-bounds.c array-bounds.c.org
>>> 32c32
>>> < # define __counted_by(member)	__attribute__((counted_by (member)))
>>> ---
>>>> # define __counted_by(member)	__attribute__((__element_count__(#member)))
>>> 34c34
>>> < # define __counted_by(member)   __attribute__((counted_by (member)))
>>> ---
>>>> # define __counted_by(member)	/* __attribute__((__element_count__(#member))) */
>>> 
>>> Then I got the following result:
>>> [opc@qinzhao-ol8u3-x86 Kees]$ ./array-bounds 2>&1 | grep -v ^'#'
>>> TAP version 13
>>> 1..12
>>> ok 1 global.fixed_size_seen_by_bdos
>>> ok 2 global.fixed_size_enforced_by_sanitizer
>>> not ok 3 global.unknown_size_unknown_to_bdos
>>> not ok 4 global.unknown_size_ignored_by_sanitizer
>>> ok 5 global.alloc_size_seen_by_bdos
>>> ok 6 global.alloc_size_enforced_by_sanitizer
>>> not ok 7 global.element_count_seen_by_bdos
>>> ok 8 global.element_count_enforced_by_sanitizer
>>> not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos
>>> not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer
>>> ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos
>>> ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer
>>> 
>>> The same as your previous results. Then I took a look at all the failed testing: 3, 4, 7, 9, and 10. And studied the reasons for all of them.
>>> 
>>> in a summary, there are two major issues:
>>> 1.  The reason for the failed testing 7 is the same issue as I observed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557
>>> Which is not a bug, it’s an expected behavior. 
>>> 
>>> 2. The common issue for  the failed testing 3, 4, 9, 10 is:
>>> 
>>> for the following annotated structure: 
>>> 
>>> ====
>>> struct annotated {
>>>        unsigned long flags;
>>>        size_t foo;
>>>        int array[] __attribute__((counted_by (foo)));
>>> };
>>> 
>>> 
>>> struct annotated *p;
>>> int index = 16;
>>> 
>>> p = malloc(sizeof(*p) + index * sizeof(*p->array));  // allocated real size 
>>> 
>>> p->foo = index + 2;  // p->foo was set by a different value than the real size of p->array as in test 9 and 10
>>> or
>>> p->foo was not set to any value as in test 3 and 4
>>> 
>>> ====
>>> 
>>> i.e, the value of p->foo is NOT synced with the number of elements allocated for the array p->array.  
>>> 
>>> I think that this should be considered as an user error, and the documentation of the attribute should include
>>> this requirement.  (In the LLVM’s RFC, such requirement was included in the programing model: 
>>> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18)
>>> 
>>> We can add a new warning option -Wcounted-by to report such user error if needed.
>>> 
>>> What’s your opinion on this?
>> 
>> 
>> Additionally, we could also have a sanitizer that
>> checks this at run-time.
> 
> Yes, that’s also a nice feature to have. 
> I think that the main point here is to catch such user errors during compilation time or run time. 
> 
> I will add one or two separate patches for these compilation warning and sanitizer feature.
> 
> 
>> 
>> Personally, I am still not very happy that in the
>> following example the two 'n's refer to different
>> entities:
>> 
>> void f(int n)
>> {
>>   struct foo {
>>       int n;   
>>       int (*p[])[n] [[counted_by(n)]];
>>   };
>> }
>> 
> Me either )-:
> 
> 
>> But I guess it will be difficult to convince everybody
>> that it would be wise to use a new syntax for
>> disambiguation:
>> 
>> void f(int n)
>> {
>>   struct foo {
>>       int n;   
>>       int (*p[])[n] [[counted_by(.n)]];
>>   };
>> }
>> 
> 
> I guess that it’s quite hard to convince everyone that the new syntax is the best solution at this moment. 
> And it might not worth the effort at this time.
> 
> We can do the new syntax later if necessary.
> 
> thanks.
> 
> Qing
> 
>> Martin
>> 
>> 
>>> 
>>> thanks.
>>> 
>>> Qing
>>> 
>>> 
>>>> On May 26, 2023, at 4:40 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> 
>>>> On Thu, May 25, 2023 at 04:14:47PM +0000, Qing Zhao wrote:
>>>>> GCC will pass the number of elements info from the attached attribute to both 
>>>>> __builtin_dynamic_object_size and bounds sanitizer to check the out-of-bounds
>>>>> or dynamic object size issues during runtime for flexible array members.
>>>>> 
>>>>> This new feature will provide nice protection to flexible array members (which
>>>>> currently are completely ignored by both __builtin_dynamic_object_size and
>>>>> bounds sanitizers).
>>>> 
>>>> Testing went pretty well, though I think I found some bdos issues:
>>>> 
>>>> - some things that bdos can't know the size of, and correctly returned
>>>> SIZE_MAX in the past, now thinks are 0-sized.
>>>> - while bdos correctly knows the size of an element_count-annotated
>>>> flexible array, it doesn't know the size of the containing object
>>>> (i.e. it returns SIZE_MAX).
>>>> 
>>>> Also, I think I found a precedence issue:
>>>> 
>>>> - if both __alloc_size and 'element_count' are in use, the _smallest_
>>>> of the two is what I would expect to be enforced by the sanitizer
>>>> and reported by __bdos. As is, alloc_size appears to be used when
>>>> it is available, regardless of what 'element_count' shows.
>>>> 
>>>> I've updated my test cases to show it more clearly, but here is the
>>>> before/after:
>>>> 
>>>> 
>>>> GCC 13 (correctly does not implement "element_count"):
>>>> 
>>>> $ ./array-bounds 2>&1 | grep -v ^'#'
>>>> TAP version 13
>>>> 1..12
>>>> ok 1 global.fixed_size_seen_by_bdos
>>>> ok 2 global.fixed_size_enforced_by_sanitizer
>>>> ok 3 global.unknown_size_unknown_to_bdos
>>>> ok 4 global.unknown_size_ignored_by_sanitizer
>>>> ok 5 global.alloc_size_seen_by_bdos
>>>> ok 6 global.alloc_size_enforced_by_sanitizer
>>>> not ok 7 global.element_count_seen_by_bdos
>>>> not ok 8 global.element_count_enforced_by_sanitizer
>>>> not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos
>>>> not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer
>>>> ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos
>>>> ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer
>>>> 
>>>> 
>>>> ToT GCC + this element_count series:
>>>> 
>>>> $ ./array-bounds 2>&1 | grep -v ^'#'
>>>> TAP version 13
>>>> 1..12
>>>> ok 1 global.fixed_size_seen_by_bdos
>>>> ok 2 global.fixed_size_enforced_by_sanitizer
>>>> not ok 3 global.unknown_size_unknown_to_bdos
>>>> not ok 4 global.unknown_size_ignored_by_sanitizer
>>>> ok 5 global.alloc_size_seen_by_bdos
>>>> ok 6 global.alloc_size_enforced_by_sanitizer
>>>> not ok 7 global.element_count_seen_by_bdos
>>>> ok 8 global.element_count_enforced_by_sanitizer
>>>> not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos
>>>> not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer
>>>> ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos
>>>> ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer
>>>> 
>>>> 
>>>> Test suite is here:
>>>> https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c
>>>> 
>>>> -- 
>>>> Kees Cook