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

[Martin Uecker <uecker@tugraz.at>]

Am Montag, dem 17.07.2023 um 16:40 -0700 schrieb Kees Cook:
> On Mon, Jul 17, 2023 at 09:17:48PM +0000, Qing Zhao wrote:
> > 
> > > On Jul 13, 2023, at 4:31 PM, Kees Cook <keescook@chromium.org>
> > > wrote:
> > > 
> > > In the bug, the problem is that "p" isn't known to be allocated,
> > > if I'm
> > > reading that correctly?
> > 
> > I think that the major point in PR109557
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109557):
> > 
> > for the following pointer p.3_1, 
> > 
> > p.3_1 = p;
> > _2 = __builtin_object_size (p.3_1, 0);
> > 
> > Question: why the size of p.3_1 cannot use the TYPE_SIZE of the
> > pointee of p when the TYPE_SIZE can be determined at compile time?
> > 
> > Answer:  From just knowing the type of the pointee of p, the
> > compiler cannot determine the size of the object.  
> 
> Why is that? "p" points to "struct P", which has a fixed size. There
> must be an assumption somewhere that a pointer is allocated,
> otherwise
> __bos would almost never work?

It often does not work, because it relies on the optimizer
propagating the information instead of the type system.

This is why it would be better to have proper *bounds* checks,
and not just object size checks. It is not quite clear to me
how BOS and bounds checking is supposed to work together,
but FAMs should be bounds checked. 

...

> 
> > 
> > > This may be
> > > desirable in a few situations. One example would be a large
> > > allocation
> > > that is slowly filled up by the program.
> > 
> > So, for such situation, whenever the allocation is filled up, the
> > field that hold the “counted_by” attribute should be increased at
> > the same time,
> > Then, the “counted_by” value always sync with the real allocation. 
> > > I.e. the counted_by member is
> > > slowly increased during runtime (but not beyond the true
> > > allocation size).
> > 
> > Then there should be source code to increase the “counted_by” field
> > whenever the allocated space increased too. 
> > > 
> > > Of course allocation size is only available in limited
> > > situations, so
> > > the loss of that info is fine: we have counted_by for everything
> > > else.
> > 
> > The point is: allocation size should synced with the value of
> > “counted_by”. LLVM’s RFC also have the similar requirement:
> > https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854#maintaining-correctness-of-bounds-annotations-18
> 
> Right, I'm saying it would be nice if __alloc_size was checked as
> well,
> in the sense that if it is available, it knows without question what
> the
> size of the allocation is. If __alloc_size and __counted_by conflict,
> the smaller of the two should be the truth.
> 
> But, as I said, if there is some need to explicitly ignore
> __alloc_size
> when __counted_by is present, I can live with it; we just need to
> document it.
> 
> If the RFC and you agree that the __counted_by variable can only ever
> be
> (re)assigned after the flex array has been (re)allocated, then I
> guess
> we'll see how it goes. :) I think most places in the kernel using
> __counted_by will be fine, but I suspect we may have cases where we
> need
> to update it like in the loop I described above. If that's true, we
> can
> revisit the requirement then. :)

It should be the other way round: You should first set
'count' and then reassign the pointer, because you can then
often check the pointer assignment (reading 'count').  The
other way round this works only sometimes, i.e. if both
assignments are close together and the optimizer can see this.



Martin