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

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

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.

Sid