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

Albert ARIBAUD
2011-09-24T09:37:07Z

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

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')

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.

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.

> Regards,
> Simon

Amicalement,
--
Albert.