Re: [PATCH] Updated buffer overflow protection patch

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Oct 27, 2004 at 12:42:58PM -0400, Diego Novillo wrote:
> On Wed, 2004-10-27 at 04:18, Jakub Jelinek wrote:
> 
> > E.g. glibc checks gets (if requested via -D_FORTIFY_SOURCE):
> > extern char *__gets_chk (char *__str, size_t);
> > #define gets(__str) \
> >   ((__bos (__str) == (size_t) -1)                               \
> >    ? (gets) (__str) : __gets_chk (__str, __bos (__str)))
> > 
> Why this hackery in the header files?  Given -ffortify-string-ops, we
> could just expand to 'gets_chk' if the length of __str is known at
> compile time.

gets is not a builtin (and not sure why it should be).  This is something
that can be trivially dealt in the headers, so the compiler doesn't need
to be involved.

> I agree with providing __builtin_object_size for users to add to their
> code.  But for the handful of builtins already known by GCC, we
> shouldn't need to force glibc to do this.  I think I'm missing something
> here.

You would need -ffortify-string-ops={1,2} to differentiate between
object and subobject sizes, plus some configury that will check if the
target libc has the __*_chk routines and if not disallow that option
(there is nothing that checks target libraries in current gcc
configury framework).
Plus people need to use -D_FORTIFY_SOURCE={1,2} anyway for other routines
and other security measures (e.g. the %n protection glibc does).

I think it is better to keep policy decision in libc's hands
(or aux library with <string.h>/<stdio.h> using #include_next),
I don't see what we would gain by hardcoding it in GCC.

> > I briefly looked at using the generic propagator but couldn't see how
> > it matches the needs of object size propagation.
> > First of all, I'd like to understand whether you want to propagate all
> > attributes in one pass or have separate passes for different attributes
> > (say VRP, string length, CCP, object size) and just use the generic
> > propagator for that.
> >
> Object size is a generic value that will be useful in other contexts
> (string length, ranges, mudflap), so I don't think it's a good idea
> computing it on demand out of __bos calls.
> 
> Once you have propagated the known object sizes for 'char *' and 'char
> []' objects, folding __bos is trivial.  

If it is not computed on-demand, then all pointers need to have object sizes
computed 4 values (maximum object size, maximum subobject size, minimum
object size, minimum subobject size).

> > The second pass deals with __builtin_object_size computing minimum
> > within DU chain loops.  For
> > char buf[64], *p;
> > p = &buf[2];
> > for (i = 0; i < x; ++i)
> >   p += 2;
> > return __builtin_object_size (p, N);
> > if N is 0 or 1 (i.e. when computing maximum) 62 should be returned, as
> > pointer arithmetic must be within the object bounds (+ one past the last
> > byte).  If N is 2 or 3 (computing minimum), 0 should be returned, but
> > getting there through pass 3 could take too long.
> >
> Hmm?  I thought the second argument for __bos was a boolean?  We have

It is not, it is an integer from 0 to 3 (well, essentially a bitmask).

> the capability of counting number of iterations in a loop.  If the loop
> is countable, you could easily deduce what would be the value of p at
> the end (Sebastian, Zdenek, is this correct?)

__bos can be used also within the loop, plus it should handle uncountable
loops as well.
The second pass also differentiates between DU loops that ever increment
the pointer and DU loops that don't increment.  The second case means
object sizes are equal for all the variables in the cycle, while the first
case need to have safe unknown minimum (and no change for maximum).

> > Pass 3 is propagation of the object sizes that still need to be
> > computed, until no more changes happen.
> > 
> These two are dealt with in the statement and PHI node visit functions. 
> What you have here is a classical value propagation problem.  The
> generic propagator is a perfect fit.  You are duplicating much of the

Well, there are just a few lines that are actually the propagator, most of
the code are the routines that compute the value or merge the information
(that needs to be done even with the generic propagator).

> propagation engine.  I'll try and come up with a prototype for this.  I

I'd appreciate that.  I'll try to help as soon as you have something.
gcc.dg/builtin-object-size-{1,2,3,4,5}.c tests cover __bos quite a lot,
so it will be easy to find out if the new implementation covers at least
all the cases as my current one and whether it covers even more things.

> want to make sure we're trying to solve the same problem.

For me the more important part is the use visible interface, how exactly
the propagator works is only an implementation detail that can be changed
in the future when needed.  __bos is always allowed to return a safe
"don't know" value, therefore in the future __bos can handle more cases
precisely.

	Jakub