[U-Boot] [PATCH 0/4] Buffer overruns in printf

[Simon Glass]

Hi Albert,

On Mon, Sep 26, 2011 at 4:20 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Hi Simon,
>
> Le 25/09/2011 16:50, Simon Glass a écrit :
>
>>> Basically, printf family functions which do not have the 'n' are *know*
>>> by
>>> all -- experienced enough :) -- programmers to be *unsafe* (but to
>>> require
>>> less from the caller) and it should remain so: no programmer should ever
>>> encounter an implementation of printf that pretends to be even somewhat
>>> safe, because it might bite him/her elsewhere, in another project based
>>> on
>>> another C library where printf is just the beartrap it usually is.
>>>
>>> IOW, programmers already have assumptions about *printf(), including how
>>> to
>>> deal with length limitations and what happens if you don't, and it is
>>> best
>>> that these assumption remain true whatever project they work with.
>>
>> I don't quite understand this. You are saying that we shouldn't modify
>> the existing printf(), serial_printf() etc. so live within the buffer
>> they use? If I call printf() and my resulting string is too long for
>> the library then I would much prefer to get truncated output than have
>> to hunt for a wierd crash.
>
> I understand the preference for truncated output, but the output length is
> primarily in the hands of the caller, through the format stringand possibly
> also through *nprintf() -- i.e. the caller can only get an output overrun if
> (s)he does not take the required steps to avoid it.

So we are talking about two things here. I am fine with this one -
basically people can call sprintf() or snprintf(). If the former then
they should be aware of the potential for overflow, if the latter they
should prepare for possible truncation.

>
>> It sounds like you are asking for a new printf(), serial_printf()
>> which provides the facility of limiting the output to n characters,
>> and leave these ones alone. But these functions are not passed a
>> buffer - they use their own and they set the site of it. So I think
>> they should be responsible for enforcing that size.
>
> The existing contract for printf family functions (think wider than U-Boot)
> does not enforce this responsibility on them.

Let me re-word: printf() and serial_printf() should be responsible for
working within the buffer that they use. In the 'existing printf()
contract' it is no concern of the user what the size of that buffer
may be. In U-Boot we have an implementation that works wholly within
that buffer, and that buffer is small, so we are exposing our
limitation, either by a buffer overflow crash or truncation. I
advocate the latter.

>
>> If you are asking for a new set of functions - nprintf(),
>> serial_nprintf(), etc. then I wonder how you can pass the 'n' but not
>> the actual buffer. In my mind you should not do one without the other.
>>
>> So please can you clarify?
>
> I should clarify indeed. My opinion, expressed as a single general rule, is
> "keep the known semantics of *printf() function family unchanged in U-Boot
> wrt all other printif implementations around". Thus I think that:
>
> - the printf() function should *not* attempt anything to mitigate length
> issues beyond what the standard mandates. If a user calls printf(), (s)he is
> expected to be aware of the risks of overruns,  and to take -- if (s)he so
> decides -- steps to avoid it; besides, the function does not have a way.
> Conversively, implementers of the printf() function need not consider any
> specific recovery action with regard to size issues. For instance, why do we
> have an internal buffer for printf to begin with? The implementation does
> not need them (and besides, I guess the buffer does not respect the single
> C99 environmental constraint for printf, that any field should be able to be
> at least 4095 bytes -- no kidding).
>
> - users who actually wisht to limit outpout ca use either
>
> -- a crafted format string with maximum-sized format specifiers, or
>
> -- a call tp a *nprintf() function into a local buffer of known size
> followed by a call to fputs().

I think this last one is clumsy. It could lead to buffers all over
U-Boot to work around the printf() limitation. If there is a desire
for this (and I'm not saying there is), let's enhance printf() to
output bit by bit as it goes (as a separate discussion from this one
though). One justification for leaving printf() as it is (but with a
size limit instead of a buffer overflow) is that people seldom printf
more than a few lines at once.

>
>>>> By the way, printf() ends up calling the same code, but without limit
>>>> checking in place. The alternative is to duplicate all the format
>>>> string processing code (a limit-checking version and an unchecked
>>>> version) which would be worse.
>>>
>>> I don't intend to dictate the way things can be implemented, so the
>>> degree
>>> of code reuse is an open question as far as I am concerned. I am only
>>> voicing my opinion that *printf() APIs and their contracts should remain
>>> identical across all implementations of *printf(), and thus that
>>> providing
>>> *nprintf() where they don't exist is commandable, but hardening printf()
>>> is
>>> not, since you basically cannot do it without somewhat departing from the
>>> de
>>> facto standard.
>>
>> OK fair enough. People are used to printf() just working, no matter
>> what the size. I haven't looked at the implementation but I suspect
>> when the buffer fills it outputs it and starts the buffer again .In
>> any case you don't have to worry about it with the GNU C library.
>
> You may also find implementations where the buffer is flushed after each
> literal part in the format string and after each format specifier output. Or
> some which emit serial characters as soon as they are produced and use a
> buffer only for those formats where the ASCII representation cannot be
> easily constructed left-to-right.
>
>> We probably don't need to go that far in U-Boot, but some simple
>> checking would avoid a nasty surprise for the user. It is obvious from
>> the result that something is truncated, and we can WARN if that helps.
>
> A user who does not expect a nasty surprise from calling printf() with a
> fair chance of overflowing the output buffer deserves the surprise. :)

For sprintf() I agree - this is well understood and people are aware
of it. For printf() I am not so sure.

Regards,
Simon

>
>> Regards,
>> Simon
>
> Amicalement,
> --
> Albert.
>