Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896)

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

> On Jan 29, 2024, at 3:19 PM, Kees Cook <keescook@chromium.org> wrote:
> 
> On Mon, Jan 29, 2024 at 07:32:06PM +0000, Qing Zhao wrote:
>> 
>> 
>>> On Jan 29, 2024, at 12:25 PM, Kees Cook <keescook@chromium.org> wrote:
>>> 
>>> On Mon, Jan 29, 2024 at 04:00:20PM +0000, Qing Zhao wrote:
>>>> An update on the kernel building with my version 4 patch.
>>>> 
>>>> Kees reported two FE issues with the current version 4 patch:
>>>> 
>>>> 1. The operator “typeof” cannot return correct type for a->array;
>>>> 2. The operator “&” cannot return correct address for a->array;
>>>> 
>>>> I fixed both in my local repository. 
>>>> 
>>>> With these additional fix.  Kernel with counted-by annotation can be built successfully. 
>>> 
>>> Thanks for the fixes!
>>> 
>>>> 
>>>> And then, Kees reported one behavioral issue with the current counted-by:
>>>> 
>>>> When the counted-by value is below zero, my current patch 
>>>> 
>>>> A. Didn’t report any warning for it.
>>>> B. Accepted the negative value as a wrapped size.
>>>> 
>>>> i.e. for:
>>>> 
>>>> struct foo {
>>>> signed char size;
>>>> unsigned char array[] __counted_by(size);
>>>> } *a;
>>>> 
>>>> ...
>>>> a->size = -3;
>>>> report(__builtin_dynamic_object_size(p->array, 1));
>>>> 
>>>> this reports 253, rather than 0.
>>>> 
>>>> And the array-bounds sanitizer doesn’t catch negative index bounds neither. 
>>>> 
>>>> a->size = -3;
>>>> report(a->array[1]); // does not trap
>>>> 
>>>> 
>>>> So, my questions are:
>>>> 
>>>> How should we handle the negative counted-by value?
>>> 
>>> Treat it as always 0-bounded: count < 0 ? 0 : count
>> 
>> Then the size of the object is 0?
> 
> That would be the purpose, yes. It's possible something else has
> happened, but it would mean "the array contents should not be accessed
> (since we don't have a valid size)".

This might be a new concept we need to add, from my understanding,
 C/C++ don’t have the zero-sized object. 
So, I am a little worried about where should we add this concept?

The most reasonable place I am thinking is adding such concept to the 
doc of “counted-by” attribute, but still not very sure on this.
> 
>> 
>>> 
>>>> 
>>>> My approach is:
>>>> 
>>>>  I think that this is a user error, the compiler need to Issue warning during runtime about this user error.
>>>> 
>>>> Since I have one remaining patch that has not been finished yet:
>>>> 
>>>> 6  Emit warnings when the user breaks the requirments for the new counted_by attribute
>>>> compilation time: -Wcounted-by
>>>> run time: -fsanitizer=counted-by
>>>>    * The initialization to the size field should be done before the first reference to the FAM field.
>>> 
>>> I would hope that regular compile-time warnings would catch this.
>> If the value is known at compile-time, then compile-time should catch it.
>> 
>>> 
>>>>    * the array has at least # of elements specified by the size field all the time during the program.
>>>>    * the value of counted-by should not be negative.
>>> 
>>> This seems reasonable for a very strict program, but it won't work for
>>> the kernel as-is: a negative "count" is sometimes used to carry failure
>>> details back to other users of the structure. This could be refactored in
>>> the kernel, but I'd prefer that even without -fsanitizer=counted-by the
>>> runtime behaviors will be "safe".
>> 
>> So, In the kernel’s source code, for example:
>> 
>> struct foo {
>>  int count;
>>  short array[] __counted_by(count);
>> };
>> 
>> The field “count” will be used for two purposes:
>> A. As the counted_by for the “array” when its value > 0;
>> B. As an errno when its value < 0;  under such condition, the size of “array” is zero. 
>> 
>> Is the understanding correct?
> 
> Yes.
> 
>> Is doing this for saving space?  (Curious -:)
> 
> It seems so, yes.
> 
>>> It does not seem sensible to me that adding a buffer size validation
>>> primitive to GCC will result in conditions where a size calculation
>>> will wrap around. I prefer no surprises. :)
>> 
>> Might be a bug here. I guess. 
>>> 
>>>> Let me know your comment and suggestions.
>>> 
>>> Clang has implemented the safety logic I'd prefer:
>>> 
>>> * __bdos will report 0 for any sizing where the "counted_by" count
>>> variable is negative. Effectively, the count variable is always
>>> processed as: count < 0 ? 0 : count
>>> 
>>> struct foo {
>>> int count;
>>> short array[] __counted_by(count);
>>> } *p;
>>> 
>>> __bdos(p->array, 1) ==> sizeof(*p->array) * (count < 0 ? 0 : count)
>> 
>> NOTE,  __bdo will use value 0 as UNKNOWN_SIZE for MINMUM SIZE query, i.e:
>> 
>> size_t __builtin_object_size (const void * ptr, int type)
>> 
>> Will return 0 as UNKNOW_SIZE when type= 2 or 3.
>> 
>> So, I am wondering: should  the 0 here is  UNKNOWN_SIZE or 0 size?
>> 
>> I guess should be the UNKNOWN_SIZE?  (I,e, -1 for MAXIMUM type,  0 for MINIMUM type).
>> 
>> i.e, when the value of “count” is 0 or negative,  the __bdos will return UNKNOWN_SIZE.  Is this correct?
> 
> I would suggest that a negative count should always return 0. The size
> isn't "unknown", the "count" has been clamped to 0 to avoid surprises,
> so the result is as if the "count" had a zero value.

There are two things here. 

1. The value of the “counted-by” is 0; (which is easy to be understood)
2. The result of the _builtin_object_size when see a “counted-by” 0.

For 1, it’s simple, if we see a counted-by value <= 0,  then counted-by is 0;

But for 2, when the _builtin_object_size sees a “counted-by” 0, what’s value it will return for the object size?

 Can we return 0 for the object size? 
(As I mentioned in the previous email, 0 in __builtin_object_size doesn’t mean size 0,
 it means UNKNOWN_SIZE when the type is 2/3, So, what’s value we should return for the size 0?)
https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html

Hope I am clear this time. -:)

thanks.

Qing
> 
>> Okay, when the value of “count” is 0 or negative, bound sanitizer will report out-of-bound (or trap) for any access to the array. 
>> This should be reasonable.
> 
> Thanks! And with __bdos() following this logic there won't be a disconnect
> between the two. i.e. FORTIFY-style things like memcpy use __bdos for
> validating the array size, and direct index walking uses the bounds
> sanitizer. These are effectively the same thing, so they need to agree.
> 
> i.e. these are the same thing:
> 
> memcpy(p->array, src, bytes_to_copy);
> 
> and
> 
> for (i = 0; i < elements_to_copy; i++)
> p->array[i] = src++
> 
> so the __bdos() used by the fortified memcpy() needs to agree with the
> logic that the bounds sanitizer uses for the for loop's accesses.
> 
> -- 
> Kees Cook