This is the mail archive of the binutils@sources.redhat.com mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: [patch] bfd/som.c: fix local buffer overrun (was: gas (binutils) 2.10: SIGSEGV on hppa1.1-hp-hpux10.20)


On Tue, 12 Sep 2000, Jeffrey A Law wrote:
> You should explain in detail why the buffer overran.

Because it was of hardwired size and too small for the input file.

+              /* Reallocate if now empty buffer still too small.  */
+              if (5 + length > tmp_space_size)
+                {
[...]
+                  tmp_space_size = MAX (2 * tmp_space_size, 5 + length);
+                  tmp_space = alloca (tmp_space_size);
+                }

Gas crashed because the buffer tmp_space in som_write_symbol_strings() was
smaller (SOM_TMP_BUFSIZE, 8192) than a single symbol (9172) in the input
file of my original bug report.  You can also reproduce it with this
single-line input file containg just that symbol (as a label).

begin 644 symbol.s.gz
M'XL("$;UO3D"`W-Y;6)O;"YS`.V7O6K#,!2%]SR-9:4TF3-T:<$XFY:+K-ZV
M`L5)I..A>?HX_7F#$#`ZHZ2+.'P</J0X%LV0:8SG246PD7X09-4GA_7)Q]PZ
MF';P)08IR''\M"[,6YO?A80OG^=Y'U',[:!=B[SKAY\2Q*=T#`(]G)*'MD,3
MFSM?]I=PMX"HMBBLZV`:(+WZRW>';-UV7NP/Y0UE$;B3EGET.4F)^S%)MS^[
M'L=,YH]*:AK9:](`4V"H0O:$*J0*JZ[XPMI!#]*#]""9W^NR%YCG_]^[1&B^
MP;=\&;(N-"*-6"7SGO)C,R@_RJ]&YAWEQV90?I1?A<S['>7'9E!^E%^%S%>K
)*UK@U"S6(P``
`
end

When gas is fed with an input file like that, the attempt to write too long
a symbol overruns the temp buffer. Any old buffer contents does get written
out because the *remaining* free space is insufficient, but after that, the
*entire* buffer is still too small, a condition which is not checked.

     bfd_put_32 (abfd, length, p);
      strings_size += 4;
      p += 4;

      /* Next comes the string itself + a null terminator.  */
      strcpy (p, syms[i]->name);

      som_symbol_data(syms[i])->stringtab_offset = strings_size;
      p += length + 1;
      strings_size += length + 1;

The strcpy corrupts its own stack frame and segfaults when it tries to
return. This is also a security hazard. An input file could be constructed
to have the assembler execute arbitrarily code when it is assembled.

Since no fixed value of SOM_TMP_BUFSIZE fits all valid input data, I fixed
it by using alloca.

Doing so, I also looked at the other places that use the same fixed size
(for different purposes!) and either applied the same fix or put in
assertions that we don't need it. 

At the same time, I also peephole-reviewed the code I was changing and
inserted comments where I found things strange. I meant that either for
people with a deeper understanding to replace my suspicions with comments
explaining why it is correct. Or to help future maintainers finding these
places in case the strangenesses do constitute bugs.

>   > I wasn't entirely sure that alloca is always safe if used together with
>   > nested scopes. 
> It is.  
> 
>   > (Especially when you do an alloca in a nested scope, leave it and
>   > enter another nested scope (as happens here): When re-using the stack space
>   > of the old scope, whether the automatic space of the new scope (if larger)
>   > might overlap with the alloca-ted area.)
> The alloca'd memory is not released until the current function exits
> (or possibly even later).  Thus the situation you mention can not cause
> problems.

OK, if you say it is safe by definition of alloca. Basically, I was trying
to make 100% sure I am not introducing a new bug. Detecting broken
alloca's is a different issue then.

Unless I am introducing a new dependency on an aspect of alloca that was
not there before.

int f ()
{
  char *dynbuf = alloca (64);
  {
    char buf[1024];
    dynbuf = alloca (8192);
  }
  {
    char smallbuf[25];
    assert (smallbuf+24 < dynbuf || dynbuf+8191 < smallbuf); 
  }
}

Would the configure script detect it if an existing native alloca failed
in this particular way, and would the alloca "simulation" written in C
would be used on the binutils host? Note that this depends on
the compiler's stack layout and that an assembler-written alloca that
normally works for the host processor would fail the same way with that
compiler. (We cannot rely on gcc being used here. Let someone want to
cross-compile for HPPA on a host to which gcc has not yet been ported.)

>   > Incidentall, isn't that fixed-sized buffer banned by GNU coding standards?
> Somewhat.  Depends on the precise situation.  I'll be able to evaluate the
> problem when you provide the analysis mentioned above :-)

Here, the fixed size buffer artificially restricts the set of valid input
files the utility can process, and that limit is not even documented to
the user (which was g++ in this case;-).

>   > +#if 100 > SOM_TMP_BUFSIZE
>   > +#error "SOM_TMP_BUFSIZE too small"
>   > +#endif
> Don't put this kind of stuff in the middle of the file.  Put it at the
> top of the file with the other definitions.

I put the assertion where the fact was relied on and where the comment
explaining the magic number 100 was. (It might have been better to give
that number a name and define it at the top near SOM_TMP_BUFSIZE.)

> This statement is completely useless anyway since there are no provisions
> for overriding the definition of SOM_TMP_BUFSIZE (which is 8k).

I only meant it to catch situations when someone experimenting with
SOM_TMP_BUFSIZE chooses too small a value.

> Avoid using #error, many non-ANSI compilers don't grok it, and BFD code
> must be compilable by those non-ANSI compilers.

We can leave it out altogether. Fine. It may have been a bit paranoic. I
just prefer assertions over comments claiming that something cannot happen.

>   > +      /* Double check it fits in the now empty buffer.  If this should
>   > +         ever fail, we will need to fix it using alloca as below.  */
>   > +      if (5 + length > SOM_TMP_BUFSIZE)
>   > +	abort ();
> Technically it is never OK to call abort from within BFD.  Technically we
> should return an error and let the application decide what to do.
> Unfortunately, som.c was written before that design decision was made and
> thus has some scattered aborts.  I'd prefer not to add new ones though by
> having new code return errors in the proper way.

Doesn't that depend on whether the error is something like "file system
full" or whether it is a bug in the code itself?

I believe the original author meant this could not happen (or they neglected
to think about this condition). Without this assertion, the code would, if
it did happen, crash in the strcpy following it, but only after corrupting
the stack so that debugging becomes difficult.

With the abort, if it did happen, a user could make a bug report from
which the maintainers could tell we did need alloca here too. If we return
false instead, gas would gracefully refuse to assemble the input file
(because that is the only thing the application can decide to do), but
without giving a clue (or even a misleading one from the last errno) and
it might never get fixed.

But maybe you can rule out anyway that this might ever happen. I don't
fully understand what is going on here. Can space names (what are those?)
become arbitrarily long because they directly correspond to names in the
input file? In that case we do need alloca and should fix that now. (An
example input file would help for that.) Or do we know a fixed upper bound
of the length of those names. Then we should document that here and maybe
put an assertion in checking that.

>   > -  memset (tmp_space, 0, SOM_TMP_BUFSIZE);
>   > +#if 0
>   > +  /* This is probably useless, as long as it is done only before the
>   > +     first filling of the buffer.  If there should be any reason to
>   > +     keep this, there will also be reason to do this each time before
>   > +     refilling the buffer, or at least after reallocating the buffer.  */
>   > +  memset (tmp_space, 0, tmp_space_size);
>   > +#endif
> Err, no it is not useless.  There are fields within the various structures
> that we do not use/need, but which must not have garbage values.  The
> buffer needs to be cleared.  

My argument was that it is either redundant here or erroneously missing in
two other places, namely where the buffer is written out and reused without
zeroing it again.

I am not sure what various structures you meant. Do functions like bfd_put_8
and bfd_put_32 leave holes or read the target buffer before they write to
it? (I am not familiar with bfd.) If not, zeroing is useless, because strcpy
does not leave holes either.

If those functions do leave holes or otherwise read the target buffer, the
buffer must be zeroed again before it is reused for another batch. In that
case we have found another bug, because that currently does not happen.

> SImilarly for other places where you removed
> the buffer initialization.

Actually, I did not remove the initialisation in the other places, I
*inserted* it. As a comment. 

Since the code seems to work without my patch (unless symbols are
excessively long), zeroing seems not needed. I entirely fail to see why it
might be needed in one place but not in the two others. So, for
consistency, I commented out the remaining place as well.

> Can you please provide the analysis I mentioned above, make the requested
> changes mentioned above, then resubmit your patch?

I would like to resubmit it only once, when this discussion has yielded some
more results and we are sure we agree on what we want to achieve.

Yet another issue, which you will have noticed when replying. The address
<pa-gdb-bugs@cs.utah.edu> (mentioned in the header of som.c) is no longer
working. Maybe you have contacts there and can confirm that. If the
failures are permanent, the address should be either removed or accompanied
by a request to send bug reports rather to bug-gnu-utils@gnu.org. (I can
include that in my eventual patch.)

> Thanks,
> jeff

Cheers,
Marco
-----------------------------------------------------------------
This email is confidential and intended solely for the use of the
individual to whom it is addressed.
Any views or opinions presented are solely those of the author
and do not necessarily represent those of Thyron Limited.
If you are not the intended recipient then please be advised
that you have received this email in error and that any use,
dissemination, forwarding, printing or copying of this email
is strictly prohibited.
If you have received this email in error, please notify the
Thyron IT Administrator on +44 (0)1923 236 050 or
send an email to mail-admin@thyron.com.
Thank You

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]