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 <keesc...@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?
--
Kees Cook
