[Skiboot] [PATCH v2 2/2] opal: Fix buffer overrun in print_* functions.

[Ananth N Mavinakayanahalli]

On Thu, Jan 15, 2015 at 12:02:06AM +0530, Mahesh J Salgaonkar wrote:
> From: Mahesh Salgaonkar <mahesh at linux.vnet.ibm.com>
> 
> While running HMI tests I saw a massive corruption in OPAL for one of the
> HMI test that injects TB error. On investigation I found that
> vsnprintf()->print_itoa() was the culprit. print_itoa function uses tmp
> array of size 16 to convert unsigned long value to ASCII. But an unsigned
> value of 0xffffffffffffffff needs atleast 25 characters to print its ASCII
> representation. This caused an array to overflow resulting into corruption,
> unpredictable behavior and finally system termination.
> 
> We could fix this by increasing the size of tmp[] array but that still won't
> fix this bug completely. While auditing vsnprintf() function I found that
> it makes use of print_*() functions to write data to buffer. But none of
> the print_* function have any check on buffer size before writing data to it.
> Without size check print_*() can easily overrun buffer passed by
> vprlog()->vsnprintf()->print_format()->print_*() causing massive corruption
> leading to unpredictable behavior.
> 
> This patch fixes this bug by modifying print_*() function to check for
> buffer size to avoid buffer overrun.
> 
> - Modified print_format(), print_fill() and print_itoa() to take bufsize
>   as argument and added a size check while writing to buffer.
> - Remove temporary array from print_itoa(), instead write data directly to
>   buffer.
> - Added two new function print_str_fill() and print_str() to be used as
>   helper routine for '%s' fmt case in print_format() function. These new
>   routines now has a check on buffer size while copying NULL terminated
>   string to buffer.
> - Added "!bufsize" check in vsnprintf() to avoid buffer overrun while
>   setting NULL character before returning.
> 
> I ran HMI tests with this patch successfully. I also tested this patch by
> reducing size of the buffer (in core/console-log.c:vprlog()) from 320 to 50
> and booted the lid successfully with no corruption at all.
> 
> Please review this patch.
> 
> Signed-off-by: Mahesh Salgaonkar <mahesh at linux.vnet.ibm.com>

Acked-by: Ananth N Mavinakayanahalli <ananth at in.ibm.com>