> 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?
>
>>
>> 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?
Is doing this for saving space? (Curious -:)
>
> 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?
>
> The logic for this is that __bdos can be _certain_ that the size is 0
> when the count variable is pathological.
>
> * -fsanitize=array-bounds similarly treats count as above, so that:
>
> printf("%d\n", p->array[index]); ==> trap when index > (count < 0 ? 0 :
> count)
>
> Same logic for the sanitizer: any access to the array when count is
> invalid means the access is invalid and must be trapped.
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.
Qing
>
>
> This means that software can run safely even in pathological conditions.
>
> -Kees
>
> --
> Kees Cook
