> On Jan 29, 2024, at 3:19 PM, Kees Cook <keesc...@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 <keesc...@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
