bus error in gcry_free

Hi,

we got some reports about segvs or bus errors with
libgcrypt.  The
bugs has been fixed yesterday.  For those who don't want to
wait for a
new release, find attached a patch.

Salam-Shalom,

   Werner


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

Werner Koch wrote:
> On Thu,  1 Feb 2007 14:34, christianbieregmx.de
said:
> 
> > This patch is nonsense. It may work if you cast to
size_t or unsigned long.
> 
> Huh?
> 
>   return (pool_okay
>           && p >= pool
>           && p < (const void*)((const
char*)pool+pool_size));
> 

If p points into the pool or just one byte after it,
everything is fine and
well-defined. Otherwise, the behaviour of this code is
completely undefined.
A compile may optimize this into this:

   return pool_okay && p != (const void*)((const
char*)pool+pool_size);

That's probably not what you meant.

> I agree that the casts are not required but they don't
harm either.
> Actually I committed this:

>   int
>   _gcry_private_is_secure (const void *p)
>   {
>     return (pool_okay
>             && p >= pool
>             && p <  pool+pool_size);
>   }

Then you've been GCCed. pointer arithmetic with "void
*" is not covered by
any C standard and usually GCC warns about this.

>   ((char*)((memblock_t*) ((char*)p - BLOCK_HEAD_SIZE))
- (char*)pool)
>    < pool_size
> 
> As you can see we can easily get an address wrap here:

Yes, that's obviously wrong too.

-- 
Christian

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu,  1 Feb 2007 14:34, christianbieregmx.de
said:

> This patch is nonsense. It may work if you cast to
size_t or unsigned long.

Huh?

  return (pool_okay
          && p >= pool
          && p < (const void*)((const
char*)pool+pool_size));

I agree that the casts are not required but they don't harm
either.
Actually I committed this:

  int
  _gcry_private_is_secure (const void *p)
  {
    return (pool_okay
            && p >= pool
            && p <  pool+pool_size);
  }
  
with 

  /* The pool of secure memory.  */
  static void *pool;
  
  /* Size of POOL in bytes.  */
  static size_t pool_size;
  
So what is the nonsense here?  This is a straightforward
check to see
whether P points into POOL.  Compare this to the old code:

  if (pool_okay && BLOCK_VALID (ADDR_TO_BLOCK (p)))

Where the second term expands to:

  ((char*)((memblock_t*) ((char*)p - BLOCK_HEAD_SIZE)) -
(char*)pool)
   < pool_size

As you can see we can easily get an address wrap here: If P
== POOL+1
and assuming a BLOCK_HEAD_SIZE of 8 we get POOL + 1 - 8 -
POOL == -7
and thus the test may or may not return false as it should. 
With
P==POOL+8 we will even get 0 < pool_size which is always
true.  The
function now falsely claims that P pointes into POOL and
will act on
it with all kinds of bad consequences.



Shalom-Salam,

   Werner


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu,  1 Feb 2007 16:24, christianbieregmx.de
said:

>>   return (pool_okay
>>           && p >= pool
>>           && p < (const void*)((const
char*)pool+pool_size));
>> 
>
> If p points into the pool or just one byte after it,
everything is fine and
> well-defined. Otherwise, the behaviour of this code is
completely undefined.
> A compile may optimize this into this:
>
>    return pool_okay && p != (const
void*)((const char*)pool+pool_size);

Sorry, I may be temporary blind but I can't see how you come
to this
conclusion.  What is wrong with:

  (const void*) ((const char*)pool + pool_size)
                 =================   =========
                  So that we can     And add the size
                  do pointer         of the pool
                  arithmetics
                ================================
                Yielding a pointer right behind POOL
  ==================================================
  Casting it to void* for proper comparing against P

?  How can the comparison be optimized away?


> Then you've been GCCed. pointer arithmetic with
"void *" is not covered by
> any C standard and usually GCC warns about this.

Damned, forgot to enable the extra gcc warnings.  I knew
that there
must have been some reason that I originally casted it. 
That happens if
you rely too much on warnings :-(


Salam-Shalom,

   Werner


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

Werner Koch wrote:
> On Thu,  1 Feb 2007 16:24, christianbieregmx.de
said:
> 
> >>   return (pool_okay
> >>           && p >= pool
> >>           && p < (const
void*)((const char*)pool+pool_size));
> >> 
> >
> > If p points into the pool or just one byte after
it, everything is fine and
> > well-defined. Otherwise, the behaviour of this
code is completely undefined.
> > A compile may optimize this into this:
> >
> >    return pool_okay && p != (const
void*)((const char*)pool+pool_size);
> 
> Sorry, I may be temporary blind but I can't see how you
come to this
> conclusion.  What is wrong with:
> 
>   (const void*) ((const char*)pool + pool_size)

You're looking at the wrong part.

> ?  How can the comparison be optimized away?

Because passing any other pointer yields either true or has
undefined behaviour.

Searching the web for the phrase

"In all other cases, the behavior is undefined."

will take you directly to relevant section of the standard.

-- 
Christian

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Thu,  1 Feb 2007 20:39, christianbieregmx.de
said:

> Because passing any other pointer yields either true or
has undefined behaviour.

You mean using pointers with relational operators.  C-99
says:


       6.5.8  Relational operators

       [#5] When two pointers are compared, the result 
depends  on
       the  relative  locations in the address space of the
objects
       pointed to.  If two pointers to object or  incomplete
 types
       both  point  to  the same object, or both point one
past the
       last element of the same array object, they  compare 
equal.
       If  the objects pointed to are members of the same
aggregate
       object, pointers to structure members declared later
compare
       greater  than  pointers  to  members declared earlier
in the
       structure,  and  pointers  to  array  elements  with 
larger
       subscript  values  compare greater than pointers to
elements
       of the same array with lower subscript values.  All
pointers
       to  members  of the same union object compare equal. 
If the
   =>  expression P points to an element of an array
object and the
   =>  expression  Q  points  to the last element of the
same array
   =>  object, the pointer expression Q+1 compares
greater than  P.
       In all other cases, the behavior is undefined.

Well, strictly interpreting you may be right.  However, this
is
irrelevant given that we assume a linear address space.  I
also doubt
that C-89 has the same requirements and that is what we code
for.

Anyway, such an interpretation of the specs is similar to
the rule
that you can't clear a structure with pointer elements by
using
memset.  Almost everyone is ignoring that and I don't want
to get
back to the time of segmented memory architectures.  



Shalom-Salam,

   Werner


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

Werner Koch wrote:
>        6.5.8  Relational operators
>    =>  expression P points to an element of an array
object and the
>    =>  expression  Q  points  to the last element of
the same array
>    =>  object, the pointer expression Q+1 compares
greater than  P.
>        In all other cases, the behavior is undefined.
 
> Well, strictly interpreting you may be right.  However,
this is
> irrelevant given that we assume a linear address space.
 I also doubt
> that C-89 has the same requirements and that is what we
code for.

GCC cannot be used as a strict C89 compiler and by default
it assumes
C99ish flavour.
 
> Anyway, such an interpretation of the specs is similar
to the rule
> that you can't clear a structure with pointer elements
by using
> memset.

Well, for float/double this isn't an esoteric issue at all.
Also similar code
can easily cause aliasing issues. I find it much cleaner to
clear structures
using struct copying using a static const variable. This
will definitely do the
right thing and isn't any more effort. Typically this
happens in one or two
places for each struct only anyway.

> Almost everyone is ignoring that and I don't want to
get
> back to the time of segmented memory architectures.

This has nothing to do with segmented memory. That's just
one example to
illustrate why such code might fail. The standard does not
define this as
"implementation-defined" in which case declaring
platforms with segmented
memory as unsupported would be fine. The standard say
"undefined behavior" and
that's emphasized by mentioning it. It would still be
"undefined behavior"
without the last sentence, as explained in some earlier
section of the
standard.

In your case you might (still) be lucky because the compiler
isn't sufficiently
smart but since everything happens in a single file with
static variables, it
would be easy to prove that pool points to a memory object
of pool_size bytes.

For example, very similar checks definitely do not work at
all with GCC 4.x:

http://www.securityfocus.com/archive/1/431184/30/0/th
readed

Casting pointers to integers and vice-versa is
implementation-defined and will
typically do exactly what you expect. So if I wanted to use
such checks at all,
I'd certainly use size_t (or unsigned long) because
arithmetic with unsigned
integers is well-defined.

-- 
Christian

_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel

On Fri,  2 Feb 2007 16:31, christianbieregmx.de
said:

> can easily cause aliasing issues. I find it much
cleaner to clear structures
> using struct copying using a static const variable.
This will definitely do the
> right thing and isn't any more effort. Typically this
happens in one or two
> places for each struct only anyway.

This break a bunch of code, be it calloc or memset cleared
structs.
But I have other things to do than to nitpicking on this.

> This has nothing to do with segmented memory. That's
just one example to

Yes sure.  The first premises is to make the code secure and
mostly
bug free.  Over-optimization of modern compilers makes this
even
harder.

> "implementation-defined" in which case
declaring platforms with segmented
> memory as unsupported would be fine. The standard say
"undefined behavior" and
> that's emphasized by mentioning it. It would still be
"undefined behavior"

Okay, granted.

> Casting pointers to integers and vice-versa is
implementation-defined and will
> typically do exactly what you expect. So if I wanted to
use such checks at all,
> I'd certainly use size_t (or unsigned long) because
arithmetic with unsigned
> integers is well-defined.

So we end up with this:

  size_t p_addr = (size_t)p;
  size_t pool_addr = (size_t)pool;

  return (pool_okay
          && p_addr >= pool_addr
          && p_addr <  pool_addr+pool_size);


Thanks,

   Werner


_______________________________________________
Gcrypt-devel mailing list
Gcrypt-develgnupg.org

http://lists.gnupg.org/mailman/listinfo/gcrypt-devel