> On Aug 7, 2023, at 12:16 PM, Kees Cook <keesc...@chromium.org> wrote:
>
> On Fri, Aug 04, 2023 at 07:44:28PM +0000, Qing Zhao wrote:
>> This is the 2nd version of the patch, per our discussion based on the
>> review comments for the 1st version, the major changes in this version
>> are:
>
> Thanks for the update!
>
>>
>> 1. change the name "element_count" to "counted_by";
>> 2. change the parameter for the attribute from a STRING to an
>> Identifier;
>> 3. Add logic and testing cases to handle anonymous structure/unions;
>> 4. Clarify documentation to permit the situation when the allocation
>> size is larger than what's specified by "counted_by", at the same time,
>> it's user's error if allocation size is smaller than what's specified by
>> "counted_by";
>> 5. Add a complete testing case for using counted_by attribute in
>> __builtin_dynamic_object_size when there is mismatch between the
>> allocation size and the value of "counted_by", the expecting behavior
>> for each case and the explanation on why in the comments.
>
> All the "normal" test cases I have are passing; this is wonderful! :)
>
> I'm still seeing unexpected situations when I've intentionally set
> counted_by to be smaller than alloc_size, but I assume it's due to not
> yet having the patch you mention below.
What’s the testing case for the one that failed?
If it’s
__builtin_dynamic_object_size(p->array, 0/2) without the allocation information
in the routine,
then with the current algorithm, gcc cannot deduce the size for the whole
object.
If not such case, let me know.
>
>> As discussed, I plan to add two more separate patch sets after this initial
>> patch set is approved and committed.
>>
>> set 1. A new warning option and a new sanitizer option for the user error
>> when the allocation size is smaller than the value of "counted_by".
>> set 2. An improvement to __builtin_dynamic_object_size for the following
>> case:
>>
>> struct A
>> {
>> size_t foo;
>> int array[] __attribute__((counted_by (foo)));
>> };
>>
>> extern struct fix * alloc_buf ();
>>
>> int main ()
>> {
>> struct fix *p = alloc_buf ();
>> __builtin_object_size(p->array, 0) == sizeof(struct A) + p->foo *
>> sizeof(int);
>> /* with the current algorithm, it’s UNKNOWN */
>> __builtin_object_size(p->array, 2) == sizeof(struct A) + p->foo *
>> sizeof(int);
>> /* with the current algorithm, it’s UNKNOWN */
>> }
>
> Should the above be bdos instead of bos?
Yes, sorry for the typo. -:)
>
>> Bootstrapped and regression tested on both aarch64 and X86, no issue.
>
> I've updated the Linux kernel's macros for the name change and done
> build tests with my first pass at "easy" cases for adding counted_by:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=devel/counted_by&id=adc5b3cb48a049563dc673f348eab7b6beba8a9b
>
> Everything is working as expected. :)
Thanks a lot for the testing!
Qing
>
> -Kees
>
> --
> Kees Cook
>
>
