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

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

>> 
>> The point is: allocation size should synced with the value of “counted_by”. LLVM’s RFC also have the similar requirement:
>> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18
> 
> Right, I'm saying it would be nice if __alloc_size was checked as well,
> in the sense that if it is available, it knows without question what the
> size of the allocation is. If __alloc_size and __counted_by conflict,
> the smaller of the two should be the truth.

I don’t think that  “if __alloc_size and __counted_by conflict, the smaller of the two should be the truth” will work correctly.

When __alloc_size is larger than the value of __counted_by, it’s okay. 
But when the value of __counted_by is larger than the __alloc_size, the array bound check or object size sanitizer might not work correctly.


Please see the following example:

struct grows {
	int alloc_count;
	int valid_count;
	int  item[] __counted_by(valid_count);
} *p;

void __attribute__((__noinline__)) something (int n)
{
	p = malloc(sizeof(*p) + sizeof(*p->item) * 100);
	p->alloc_count = 100;
	p->valid_count = 102;
	p->item[n] = 10;		// both _alloc_size and the value of __counted_by are available in this routine, the smaller one is , 100;
                    
}

void __attribute__((__noinline__))  something_2 (int n)
{
   p->item[n] = 10;   // only the value of  __counted_by is available in this routine, which is 102;  
}

Int main
{
   Something (101);
   Something_2 (101);
}


For the above example, the out-of-bound array access in routine “something” should be able to be caught by the compiler.
However, the out-of-bound array access in the routine “something_2” will NOT be able to be caught by the compiler.

Since in the routine “something_2” , the compiler don’t know the alloc_size, the only available info is the counted_by value
 through the attribute.  But this value is bigger than the REAL size of the array. Therefore the compiler cannot detect the 
out-of-bound array access in the routine something_2


Based on the above observation, I think we should add the following requirement: 

The value of “counted_by” should be equal or SMALLER than the real alloc_size for the flexible array member. 

This is the same requirement as the LLVM RFC. 
https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18

"the compiler inserts additional checks to ensure the new buf has at least as many elements as the new count indicates.”
LLVM has additional requirement in addition to this, we might need to consider those requirement too. 

Qing

> But, as I said, if there is some need to explicitly ignore __alloc_size
> when __counted_by is present, I can live with it; we just need to
> document it.
> 
> If the RFC and you agree that the __counted_by variable can only ever be
> (re)assigned after the flex array has been (re)allocated, then I guess
> we'll see how it goes. :) I think most places in the kernel using
> __counted_by will be fine, but I suspect we may have cases where we need
> to update it like in the loop I described above. If that's true, we can
> revisit the requirement then. :)
> 
> -Kees
> 
> -- 
> Kees Cook