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

[Albert ARIBAUD]

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.

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.