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

[Kees Cook <keescook@chromium.org>]

On Mon, Oct 23, 2023 at 09:57:45PM +0200, Martin Uecker wrote:
> Am Montag, dem 23.10.2023 um 12:52 -0700 schrieb Kees Cook:
> > On Fri, Oct 20, 2023 at 09:54:05PM +0200, Martin Uecker wrote:
> > > Am Freitag, dem 20.10.2023 um 18:48 +0000 schrieb Qing Zhao:
> > > > 
> > > > > On Oct 20, 2023, at 2:34 PM, Kees Cook <keescook@chromium.org> wrote:
> > > > > 
> > > > > On Fri, Oct 20, 2023 at 11:50:11AM +0200, Martin Uecker wrote:
> > > > > > Am Donnerstag, dem 19.10.2023 um 16:33 -0700 schrieb Kees Cook:
> > > > > > > On Wed, Oct 18, 2023 at 09:11:43PM +0000, Qing Zhao wrote:
> > > > > > > > As I replied to Martin in another email, I plan to do the following to resolve this issue:
> > > > > > > > 
> > > > > > > > 1. No specification for signed or unsigned for counted_by field.
> > > > > > > > 2. Add a sanitizer option -fsanitize=counted-by-bound to catch the cases when the size of the counted-by is not positive.
> > > > > > > 
> > > > > > > I don't understand why this needs to be a runtime sanitizer. The
> > > > > > > signedness is known at compile time, so I would expect a -W option.
> > > > > > 
> > > > > > The signedness of the type but not of the value.
> > > > > > 
> > > > > > But I would not want to have a warning for signed 
> > > > > > counter  types by default because I would prefer
> > > > > > to use signed types (for various reasons including
> > > > > > better overflow detection).
> > > > > > 
> > > > > > > Or
> > > > > > > do you mean you'd split up -fsanitize=bounds between unsigned and signed
> > > > > > > indexes? I'd find that kind of awkward for the kernel... but I feel like
> > > > > > > I've misunderstood something. :)
> > > > > > > 
> > > > > > > -Kees
> > > > > > 
> > > > > > The idea would be to detect at run-time the case
> > > > > > if  x->buf  is used at a time where   x->counter 
> > > > > > is negative and also when x->counter * sizeof(x->buf[0])
> > > > > > overflows or is too big.
> > > > > > 
> > > > > > This would be similar to
> > > > > > 
> > > > > > int a[n];
> > > > > > 
> > > > > > where it is detected at run-time if n is not-positive.
> > > > > 
> > > > > Right. I guess what I mean to say is that I would expect this case to
> > > > > already be caught by -fsanitize=bounds -- I don't see a reason to add an
> > > > > additional sanitizer option.
> > > > > 
> > > > > struct foo {
> > > > > 	int count;
> > > > > 	int array[] __counted_by(count);
> > > > > };
> > > > > 
> > > > > 	foo->count = 5;
> > > > > 	foo->array[0] = 1;	// ok
> > > > > 	foo->array[10] = 1;	// -fsanitize=bounds will catch this
> > > > > 	foo->array[-10] = 1;	// -fsanitize=bounds will catch this too
> > > > > 
> > > > > 
> > > > 
> > > > just checked this testing case with my GCC, and YES, -fsanitize=bounds indeed caught this error:
> > > > 
> > > > ttt_1.c:31:12: runtime error: index 10 out of bounds for type 'char [*]'
> > > > ttt_1.c:32:12: runtime error: index -10 out of bounds for type 'char [*]’
> > > > 
> > > 
> > > Yes, but I thought we were discussing the case where count is
> > > set to a negative value:
> > > 
> > > foo->count = -1;
> > > int x = foo->array[3]; // UBSan should diagnose this
> > 
> > Oh right, I keep thinking about it backwards.
> > 
> > Yeah, we can't trap the "count" assignment, because it may be getting used
> > for other purposes. But yeah, access to "array" should trap if "count"
> > is negative.
> > 
> > > And also the case when foo->array becomes too big.
> > 
> > How do you mean?
> 
> count * sizeof(member) could overflow or otherwise be
> bigger than allowed.

Ah! Yes.

foo->count = SIZE_MAX;
foo->array[0]; // UBSan diagnose:
               // SIZE_MAX * sizeof(int) is larger than can be represented

> 
> Martin
> 
> 

-- 
Kees Cook