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

[Qing Zhao <qing.zhao@oracle.com>]

> On Oct 6, 2023, at 4:01 PM, Martin Uecker <uecker@tugraz.at> wrote:
> 
> Am Freitag, dem 06.10.2023 um 06:50 -0400 schrieb Siddhesh Poyarekar:
>> On 2023-10-06 01:11, Martin Uecker wrote:
>>> Am Donnerstag, dem 05.10.2023 um 15:35 -0700 schrieb Kees Cook:
>>>> On Thu, Oct 05, 2023 at 04:08:52PM -0400, Siddhesh Poyarekar wrote:
>>>>> 2. How would you handle signedness of the size field?  The size gets
>>>>> converted to sizetype everywhere it is used and overflows/underflows may
>>>>> produce interesting results.  Do you want to limit the types to unsigned or
>>>>> do you want to add a disclaimer in the docs?  The former seems like the
>>>>> *right* thing to do given that it is a new feature; best to enforce the
>>>>> cleaner habit at the outset.
>>>> 
>>>> The Linux kernel has a lot of "int" counters, so the goal is to catch
>>>> negative offsets just like too-large offsets at runtime with the sanitizer
>>>> and report 0 for __bdos. Refactoring all these to be unsigned is going
>>>> to take time since at least some of them use the negative values as
>>>> special values unrelated to array indexing. :(
>>>> 
>>>> So, perhaps if unsigned counters are worth enforcing, can this be a
>>>> separate warning the kernel can turn off initially?
>>>> 
>>> 
>>> I think unsigned counters are much more problematic than signed ones
>>> because wraparound errors are more difficult to find.
>>> 
>>> With unsigned you could potentially diagnose wraparound, but only if we
>>> add -fsanitize=unsigned-overflow *and* add mechanism to mark intentional
>>> wraparound *and* everybody adds this annotation after carefully screening
>>> their code *and* rewriting all operations such as (counter - 3) + 5
>>> where the wraparound in the intermediate expression is harmless.
>>> 
>>> For this reason, I do not think we should ever enforce some rule that
>>> the counter has to be unsigned.
>>> 
>>> What we could do, is detect *storing* negative values into the
>>> counter at run-time using UBSan. (but if negative values are
>>> used for special cases, one also should be able to turn this
>>> off).
>> 
>> All of the object size detection relies on object sizes being sizetype. 
>> The closest we could do with that is detect (sz != SIZE_MAX && sz > 
>> size_t / 2), since allocators typically cannot allocate more than 
>> SIZE_MAX / 2.
> 
> I was talking about the counter in:
> 
> struct {
>  int counter;
>  char buf[] __counted_by__((counter))
> };
> 
> which could be checked to be positive either when stored to or 
> when buf is used.
> 
> And yes, we could also check the size of buf.  Not sure what is
> done for VLAs now, but I guess it could be similar.
> 
For VLAs, the bounds expression could be both signed or unsigned. 
But we have added a sanitizer option -fsanitize=vla-bound to catch the cases when the size of the VLA is not positive.

For example:

opc@qinzhao-ol8u3-x86 Martin]$ cat t3.c
#include <stdio.h>
size_t foo(int m)
{
  char t[m];

  return sizeof(t);
}

int main()
{
  printf ("the sizeof flexm is %lu \n", foo(-100000000));
  return 0;
}
[opc@qinzhao-ol8u3-x86 Martin]$ sh t
/home/opc/Install/latest-d/bin/gcc -fsanitize=undefined -O2 -Wall -Wpedantic t3.c
t3.c:4:8: runtime error: variable length array bound evaluates to non-positive value -100000000
the sizeof flexm is 18446744073609551616 


We can do the same thing for “counted_by”. i.e:

1. No specification for signed or unsigned for counted_by field.
2. Add an sanitizer option -fsanitize=counted-by-bound to catch the cases when the size of the counted-by is not positive.

Is this good enough?

Qing
> Best,
> Martin
> 
> 
> 
>> 
>> Sid