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

[Kees Cook <keescook@chromium.org>]

On Thu, Jul 06, 2023 at 06:56:21PM +0000, Qing Zhao wrote:
> Hi, Kees,
> 
> I have updated my V1 patch with the following changes:
> A. changed the name to "counted_by"
> B. changed the argument from a string to an identifier
> C. updated the documentation and testing cases accordingly.

Sounds great!

> 
> And then used this new gcc to test https://github.com/kees/kernel-tools/blob/trunk/fortify/array-bounds.c (with the following change)
> [opc@qinzhao-ol8u3-x86 Kees]$ !1091
> diff array-bounds.c array-bounds.c.org
> 32c32
> < # define __counted_by(member)	__attribute__((counted_by (member)))
> ---
> > # define __counted_by(member)	__attribute__((__element_count__(#member)))
> 34c34
> < # define __counted_by(member)   __attribute__((counted_by (member)))
> ---
> > # define __counted_by(member)	/* __attribute__((__element_count__(#member))) */
> 
> Then I got the following result:
> [opc@qinzhao-ol8u3-x86 Kees]$ ./array-bounds 2>&1 | grep -v ^'#'
> TAP version 13
> 1..12
> ok 1 global.fixed_size_seen_by_bdos
> ok 2 global.fixed_size_enforced_by_sanitizer
> not ok 3 global.unknown_size_unknown_to_bdos
> not ok 4 global.unknown_size_ignored_by_sanitizer
> ok 5 global.alloc_size_seen_by_bdos
> ok 6 global.alloc_size_enforced_by_sanitizer
> not ok 7 global.element_count_seen_by_bdos
> ok 8 global.element_count_enforced_by_sanitizer
> not ok 9 global.alloc_size_with_smaller_element_count_seen_by_bdos
> not ok 10 global.alloc_size_with_smaller_element_count_enforced_by_sanitizer
> ok 11 global.alloc_size_with_bigger_element_count_seen_by_bdos
> ok 12 global.alloc_size_with_bigger_element_count_enforced_by_sanitizer
> 
> The same as your previous results. Then I took a look at all the failed testing: 3, 4, 7, 9, and 10. And studied the reasons for all of them.
> 
>  in a summary, there are two major issues:
> 1.  The reason for the failed testing 7 is the same issue as I observed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557
> Which is not a bug, it’s an expected behavior. 

In the bug, the problem is that "p" isn't known to be allocated, if I'm
reading that correctly? I'm not sure this is a reasonable behavior, but
let me get into the specific test, which looks like this:

TEST(counted_by_seen_by_bdos)
{
        struct annotated *p;
        int index = MAX_INDEX + unconst;

        p = alloc_annotated(index);

        REPORT_SIZE(p->array);
/* 1 */ EXPECT_EQ(sizeof(*p), offsetof(typeof(*p), array));
        /* Check array size alone. */
/* 2 */ EXPECT_EQ(__builtin_object_size(p->array, 1), SIZE_MAX);
/* 3 */ EXPECT_EQ(__builtin_dynamic_object_size(p->array, 1), p->foo * sizeof(*p->array));
        /* Check check entire object size. */
/* 4 */ EXPECT_EQ(__builtin_object_size(p, 1), SIZE_MAX);
/* 5 */ EXPECT_EQ(__builtin_dynamic_object_size(p, 1), sizeof(*p) + p->foo * sizeof(*p->array));
}

Test 1 trivially passes -- this is just a sanity check.

Test 2 should be SIZE_MAX because __bos only knows compile-time sizes.

Test 3 should pass: counted_by knows the allocation size and can be
queried as part of the "p" object.

Test 4 should be SIZE_MAX because __bos only knows compile-time sizes.

Test 5 should pass as well, since, again, p can be examined. Passing p
to __bdos implies it is allocated and the __counted_by annotation can be
examined.

If "return p->array[index];" would be caught by the sanitizer, then
it follows that __builtin_dynamic_object_size(p, 1) must also know the
size. i.e. both must examine "p" and its trailing flexible array and
__counted_by annotation.

> 
> 2. The common issue for  the failed testing 3, 4, 9, 10 is:
> 
> for the following annotated structure: 
> 
> ====
> struct annotated {
>         unsigned long flags;
>         size_t foo;
>         int array[] __attribute__((counted_by (foo)));
> };
> 
> 
> struct annotated *p;
> int index = 16;
> 
> p = malloc(sizeof(*p) + index * sizeof(*p->array));  // allocated real size 
> 
> p->foo = index + 2;  // p->foo was set by a different value than the real size of p->array as in test 9 and 10

Right, tests 9 and 10 are checking that the _smallest_ possible value of
the array is used. (There are two sources of information: the allocation
size and the size calculated by counted_by. The smaller of the two
should be used when both are available.)

> or
> p->foo was not set to any value as in test 3 and 4

For tests 3 and 4, yes, this was my mistake. I have fixed up the tests
now. Bill noticed the same issue. Sorry for the think-o!

> ====
> 
> i.e, the value of p->foo is NOT synced with the number of elements allocated for the array p->array.  
> 
> I think that this should be considered as an user error, and the documentation of the attribute should include
> this requirement.  (In the LLVM’s RFC, such requirement was included in the programing model: 
> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18)
> 
> We can add a new warning option -Wcounted-by to report such user error if needed.
> 
> What’s your opinion on this?

I think it is correct to allow mismatch between allocation and
counted_by as long as only the least of the two is used. This may be
desirable in a few situations. One example would be a large allocation
that is slowly filled up by the program. I.e. the counted_by member is
slowly increased during runtime (but not beyond the true allocation size).

Of course allocation size is only available in limited situations, so
the loss of that info is fine: we have counted_by for everything else.

-- 
Kees Cook