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

[Simon Glass]

Hi Albert,

On Sun, Sep 25, 2011 at 1:40 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Le 24/09/2011 16:00, Simon Glass a écrit :
>
>>> So basically the choice is between:
>>>
>>> - adding code to the printf() family to try and fix an issue that it is
>>> fundamentally unable to properly fix, and for which a solution exists, or
>>>
>>> - grepping and fixing calls to *sprintf() in U-Boot that do not respect
>>> the
>>> known contraints of printf(), by resizing the buffer or calling
>>> *snprintf()
>>> instead.
>>>
>>> I am definitely not in favor of the first option concerning U-Boot.
>>
>> Sounds fine to me. So I think we need the nprintf() variants in there,
>> but the message is not to use them willy nilly. Going back to my patch
>> series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that
>> right?
>
> It is the exact opposite for me : 3/4 makes all printf functions work like
> some kind of *nprintf(), while 4/4 is about the network code switching to
> *nprintf() for safety, so 3/4 would be nak and 4/4 ack as far as I am
> concerned.
>
> 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.

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.

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?

>
>> 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.

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.

Regards,
Simon

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