> On Oct 20, 2023, at 3:54 PM, Martin Uecker <uec...@tugraz.at> 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 > > And also the case when foo->array becomes too big.
Oops, yes, you are right. Thanks. Qing > > Martin