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

[Simon Glass]

Hi Albert,

On Sat, Sep 24, 2011 at 2:37 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Le 23/09/2011 22:46, Simon Glass a écrit :
>>
>> Hi Albert,
>>
>> On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD
>> <albert.u.boot at aribaud.net>  wrote:
>>>
>>> Hi Simon,
>>>
>>> Le 23/09/2011 19:38, Simon Glass a écrit :
>>>>
>>>> The printf family of functions in U-Boot cannot deal with a situation
>>>> where
>>>> the caller provides a buffer which turns out to be too small for the
>>>> format
>>>> string. This can result in buffer overflows, stack overflows and other
>>>> bad
>>>> behavior.
>>>
>>> Indeed overruns can lead to bad behaviors, but in any case, it can never
>>> be
>>> recovered, because at the root, the problem is that the caller provided
>>> inconsistent arguments to printf.
>>
>> Recovery is one thing, but I would settle for just not crashing, which
>> is the purpose of this patch. We could also easily WARN if that were
>> considered appropriate here.
>>
>>>
>>> So in essence, you're 'fixing' printf for a design error in printf's
>>> caller,
>>> instead of fixing the design error.
>>
>> Well, the nature of a function is that it cannot know what arguments
>> might be passed to it. It can only assert(), limit check, etc. A limit
>> check is what this patch aims to add.
>
> I do agree that as a rule, a function should check its arguments, but as any
> rule, this one has understated assumptions. Here, one assumption is that
> calls to the functions cannot be made compliant, and therefore it falls to
> the function to ensure this compliance; and another one is that the function
> can actually check this conformance.

There is also the issue of programmer error, or unexpected situations,
and graceful failure in these cases. We want to avoid hard-to-find
memory/stack corruption bugs if the cost of doing so is moderate.

>
> The first assumption is correct in an OS or general-purpose library where
> the number of callers is virtually unlimited and there is no way to make
> sure all calls are conforming.
>
> In U-Boot however, the number of callers is bound and known (unless you're
> thinking of standalone U-Boot apps, but I don't see this as a use case so
> frequent that it warrants a 'fix')

Yes, and one can inspect each call and decide if the nprint() variants
are preferable in each case.
>
> The second assumption is false by nature for printf(), which simply cannot
> check its input buffer (OTOH *nprintf() does, and it is its job).
>
> Besides, there *is* a sub-family of printf functions which are dedicated to
> the case where buffers provided may be too small for the print output --
> meaning that the issue is known and an easy fix exists if cropping the
> output is allowable.

Yes, or even if cropping is not allowed, we might expect a nice error,
rather than a crash sometime later when the stack corruption or data
space overwriting causes problems.
>
> 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?

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.

> Amicalement,
> --
> Albert.
>

Regards,
Simon