On Mon, Jan 29, 2024 at 07:32:06PM +0000, Qing Zhao wrote:
>
>
> > 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?
That would be the purpose, yes. It's possible something else has
happened, but it would mean "the array contents should not be accessed
(since we don't have a valid size)".
>
> >
> >>
> >> 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?
Yes.
> Is doing this for saving space? (Curious -:)
It seems so, yes.
> > 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?
I would suggest that a negative count should always return 0. The size
isn't "unknown", the "count" has been clamped to 0 to avoid surprises,
so the result is as if the "count" had a zero value.
> 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.
Thanks! And with __bdos() following this logic there won't be a disconnect
between the two. i.e. FORTIFY-style things like memcpy use __bdos for
validating the array size, and direct index walking uses the bounds
sanitizer. These are effectively the same thing, so they need to agree.
i.e. these are the same thing:
memcpy(p->array, src, bytes_to_copy);
and
for (i = 0; i < elements_to_copy; i++)
p->array[i] = src++
so the __bdos() used by the fortified memcpy() needs to agree with the
logic that the bounds sanitizer uses for the for loop's accesses.
--
Kees Cook
