On Thu, Apr 24, 2025 at 4:56 PM Kees Cook <k...@kernel.org> wrote:
> On April 24, 2025 1:44:23 PM PDT, Qing Zhao <qing.z...@oracle.com> wrote:
> >> On Apr 24, 2025, at 15:43, Bill Wendling <isanb...@gmail.com> wrote:
> >>
> >> On Thu, Apr 24, 2025 at 8:15 AM Qing Zhao <qing.z...@oracle.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> Kees reported a segmentation failure when he used the patch to compiler
> >>> kernel,
> >>> and the reduced the testing case is something like the following:
> >>>
> >>> struct f {
> >>> void *g __attribute__((__counted_by__(h)));
> >>> long h;
> >>> };
> >>>
> >>> extern struct f *my_alloc (int);
> >>>
> >>> int i(void) {
> >>> struct f *iov = my_alloc (10);
> >>> int *j = (int *)iov->g;
> >>> return __builtin_dynamic_object_size(iov->g, 0);
> >>> }
> >>>
> >>> Basically, the problem is relating to the pointee type of the pointer
> >>> array being “void”,
> >>> As a result, the element size of the array is not available in the IR.
> >>> Therefore segmentation
> >>> fault when calculating the size of the whole object.
> >>>
> >>> Although it’s easy to fix this segmentation failure, I am not quite sure
> >>> what’s the best
> >>> solution to this issue:
> >>>
> >>> 1. Reject such usage of “counted_by” in the very beginning by reporting
> >>> warning to the
> >>> User, and delete the counted_by attribute from the field.
> >>>
> >>> Or:
> >>>
> >>> 2. Accept such usage, but issue warnings when calculating the object_size
> >>> in Middle-end.
> >>>
> >>> Personally, I prefer the above 1 since I think that when the pointee type
> >>> is void, we don’t know
> >>> The type of the element of the pointer array, there is no way to decide
> >>> the size of the pointer array.
> >>>
> >>> So, the counted_by information is not useful for the
> >>> __builtin_dynamic_object_size.
> >>>
> >>> But I am not sure whether the counted_by still can be used for bound
> >>> sanitizer?
> >>>
> >>> Thanks for suggestions and help.
> >>>
> >> Clang supports __sized_by that can handle a 'void *', where it defaults to
> >> 'u8'.
>
> I would like to be able to use counted_by (and not sized_by) so that users of
> the annotation don't need to have to change the marking just because it's
> "void *". Everything else operates on "void *" as if it were u8 ...
>
I'll float this idea past the Clang people. I don't have any immediate
objections to it.
-bw
> Regardless, ignoring "void *", the rest of my initial testing (of both GCC
> and Clang) is positive. The test cases are all behaving as expected! Yay! :)
> I will try to construct some more goofy stuff to find more corner cases.
>
> And at some future point we may want to think about
> -fsanitize=pointer-overflow using this information too, to catch arithmetic
> and increments past the bounds...
>
> struct foo {
> u8 *buf __counted_by(len);
> int len;
> } str;
> u8 *walk;
> str->buf = malloc(10);
> str->len = 10;
>
> walk = str->buf + 12; // trip!
> for (walk = str->buf; ; walk++) // trip after 10 loops
> ;
>
>
> -Kees
>
> --
> Kees Cook
