From patchwork Mon Jan 12 19:25:26 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mahesh J Salgaonkar X-Patchwork-Id: 428095 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 520F9140272 for ; Tue, 13 Jan 2015 06:26:39 +1100 (AEDT) Received: from ozlabs.org (ozlabs.org [103.22.144.67]) by lists.ozlabs.org (Postfix) with ESMTP id 0D04F1A0313 for ; Tue, 13 Jan 2015 06:26:39 +1100 (AEDT) X-Original-To: skiboot@lists.ozlabs.org Delivered-To: skiboot@lists.ozlabs.org Received: from e23smtp04.au.ibm.com (e23smtp04.au.ibm.com [202.81.31.146]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 87B2D1A0D1B for ; Tue, 13 Jan 2015 06:25:45 +1100 (AEDT) Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 13 Jan 2015 05:25:42 +1000 Received: from d23dlp03.au.ibm.com (202.81.31.214) by e23smtp04.au.ibm.com (202.81.31.210) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 13 Jan 2015 05:25:41 +1000 Received: from d23relay06.au.ibm.com (d23relay06.au.ibm.com [9.185.63.219]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id 763123578023 for ; Tue, 13 Jan 2015 06:25:40 +1100 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay06.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t0CJPd7A51118290 for ; Tue, 13 Jan 2015 06:25:40 +1100 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t0CJPdYN020294 for ; Tue, 13 Jan 2015 06:25:39 +1100 Received: from [192.168.0.3] ([9.79.218.182]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id t0CJPVYf020227; Tue, 13 Jan 2015 06:25:37 +1100 From: Mahesh J Salgaonkar To: Stewart Smith , skiboot list , Benjamin Herrenschmidt Date: Tue, 13 Jan 2015 00:55:26 +0530 Message-ID: <20150112192412.12078.15335.stgit@mars> User-Agent: StGit/0.17-dirty MIME-Version: 1.0 X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15011219-0013-0000-0000-000000AADE04 Cc: Vaidyanathan Srinivasan Subject: [Skiboot] [PATCH] opal: Fix buffer overrun in print_* functions. X-BeenThere: skiboot@lists.ozlabs.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Mailing list for skiboot development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: skiboot-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org Sender: "Skiboot" From: Mahesh Salgaonkar While running HMI tests I saw 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_*() functions 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 the 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 --- libc/stdio/vsnprintf.c | 163 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 108 insertions(+), 55 deletions(-) diff --git a/libc/stdio/vsnprintf.c b/libc/stdio/vsnprintf.c index b2f0b94..2355f8b 100644 --- a/libc/stdio/vsnprintf.c +++ b/libc/stdio/vsnprintf.c @@ -22,59 +22,99 @@ static const unsigned long long convert[] = { 0xFFFFFFFFFFULL, 0xFFFFFFFFFFFFULL, 0xFFFFFFFFFFFFFFULL, 0xFFFFFFFFFFFFFFFFULL }; +static int +print_str_fill(char **buffer, size_t bufsize, char *sizec, + const char *str, char c) +{ + int i, sizei, len; + char *bstart = *buffer; + sizei = strtoul(sizec, NULL, 10); + len = strlen(str); + if (sizei > len) { + for (i = 0; + (i < (sizei - len)) && ((*buffer - bstart) < bufsize); + i++) { + **buffer = c; + *buffer += 1; + } + } + return 1; +} static int -print_itoa(char **buffer, unsigned long value, unsigned short base, bool upper) +print_str(char **buffer, size_t bufsize, const char *str) +{ + char *bstart = *buffer; + int i; + + for (i = 0; (i < strlen(str)) && ((*buffer - bstart) < bufsize); i++) { + **buffer = str[i]; + *buffer += 1; + } + return 1; +} + +static unsigned int __attrconst +print_intlen(unsigned long value, unsigned short int base) +{ + int i = 0; + + while (value > 0) { + value /= base; + i++; + } + if (i == 0) + i = 1; + return i; +} + +static int +print_itoa(char **buffer, size_t bufsize, unsigned long value, + unsigned short base, bool upper) { const char zeichen[] = {'0','1','2','3','4','5','6','7','8','9','a','b','c','d','e','f'}; char c; - int i = 0; - char tmp[16]; + int i, len; if(base <= 2 || base > 16) return 0; + len = i = print_intlen(value, base); + + /* Don't print to buffer if bufsize is not enough. */ + if (len > bufsize) + return 0; + do { c = zeichen[value % base]; if (upper) c = toupper(c); - tmp[i++] = c; + + (*buffer)[--i] = c; value /= base; } while(value); - while (i--) { - **buffer = tmp[i]; - *buffer += 1; - } + *buffer += len; return 1; } -static unsigned int __attrconst -print_intlen(unsigned long value, unsigned short int base) -{ - int i = 0; - - while(value > 0) { - value /= base; - i++; - } - if(i == 0) i = 1; - return i; -} - static int -print_fill(char **buffer, char *sizec, unsigned long size, unsigned short int base, char c, int optlen) +print_fill(char **buffer, size_t bufsize, char *sizec, unsigned long size, + unsigned short int base, char c, int optlen) { int i, sizei, len; + char *bstart = *buffer; sizei = strtoul(sizec, NULL, 10); len = print_intlen(size, base) + optlen; - if(sizei > len) { - for(i = 0; i < (sizei - len); i++) { + if (sizei > len) { + for (i = 0; + (i < (sizei - len)) && ((*buffer - bstart) < bufsize); + i++) { **buffer = c; *buffer += 1; } @@ -85,10 +125,10 @@ print_fill(char **buffer, char *sizec, unsigned long size, unsigned short int ba static int -print_format(char **buffer, const char *format, void *var) +print_format(char **buffer, size_t bufsize, const char *format, void *var) { - unsigned long start; - unsigned int i = 0, sizei = 0, len = 0, length_mod = sizeof(int); + char *start; + unsigned int i = 0, length_mod = sizeof(int); unsigned long value = 0; unsigned long signBit; char *form, sizec[32]; @@ -96,7 +136,7 @@ print_format(char **buffer, const char *format, void *var) bool upper = false; form = (char *) format; - start = (unsigned long) *buffer; + start = *buffer; form++; if(*form == '0' || *form == '.') { @@ -104,7 +144,7 @@ print_format(char **buffer, const char *format, void *var) form++; } - while(*form != '\0') { + while ((*form != '\0') && ((*buffer - start) < bufsize)) { switch(*form) { case 'u': case 'd': @@ -117,53 +157,54 @@ print_format(char **buffer, const char *format, void *var) *buffer += 1; value = (-(unsigned long)value) & convert[length_mod]; } - print_fill(buffer, sizec, value, 10, sign, 0); - print_itoa(buffer, value, 10, upper); + print_fill(buffer, bufsize - (*buffer - start), + sizec, value, 10, sign, 0); + print_itoa(buffer, bufsize - (*buffer - start), + value, 10, upper); break; case 'X': upper = true; case 'x': sizec[i] = '\0'; value = (unsigned long) var & convert[length_mod]; - print_fill(buffer, sizec, value, 16, sign, 0); - print_itoa(buffer, value, 16, upper); + print_fill(buffer, bufsize - (*buffer - start), + sizec, value, 16, sign, 0); + print_itoa(buffer, bufsize - (*buffer - start), + value, 16, upper); break; case 'O': case 'o': sizec[i] = '\0'; value = (long int) var & convert[length_mod]; - print_fill(buffer, sizec, value, 8, sign, 0); - print_itoa(buffer, value, 8, upper); + print_fill(buffer, bufsize - (*buffer - start), + sizec, value, 8, sign, 0); + print_itoa(buffer, bufsize - (*buffer - start), + value, 8, upper); break; case 'p': sizec[i] = '\0'; - print_fill(buffer, sizec, (unsigned long) var, 16, ' ', 2); - **buffer = '0'; - *buffer += 1; - **buffer = 'x'; - *buffer += 1; - print_itoa(buffer,(unsigned long) var, 16, upper); + print_fill(buffer, bufsize - (*buffer - start), + sizec, (unsigned long) var, 16, ' ', 2); + print_str(buffer, bufsize - (*buffer - start), + "0x"); + print_itoa(buffer, bufsize - (*buffer - start), + (unsigned long) var, 16, upper); break; case 'c': sizec[i] = '\0'; - print_fill(buffer, sizec, 1, 10, ' ', 0); + print_fill(buffer, bufsize - (*buffer - start), + sizec, 1, 10, ' ', 0); **buffer = (unsigned long) var; *buffer += 1; break; case 's': sizec[i] = '\0'; - sizei = strtoul(sizec, NULL, 10); - len = strlen((char *) var); - if(sizei > len) { - for(i = 0; i < (sizei - len); i++) { - **buffer = ' '; - *buffer += 1; - } - } - for(i = 0; i < strlen((char *) var); i++) { - **buffer = ((char *) var)[i]; - *buffer += 1; - } + print_str_fill(buffer, + bufsize - (*buffer - start), sizec, + (char *) var, ' '); + + print_str(buffer, bufsize - (*buffer - start), + (char *) var); break; case 'l': form++; @@ -210,6 +251,16 @@ vsnprintf(char *buffer, size_t bufsize, const char *format, va_list arg) bstart = buffer; ptr = (char *) format; + /* + * Return from here if size passed is zero, otherwise we would + * overrun buffer while setting NULL character at the end. + */ + if (!bufsize) + return 0; + + /* Leave one space for NULL character */ + bufsize--; + while(*ptr != '\0' && (buffer - bstart) < bufsize) { if(*ptr == '%') { @@ -228,7 +279,9 @@ vsnprintf(char *buffer, size_t bufsize, const char *format, va_list arg) if(*ptr == '%') { *buffer++ = '%'; } else { - print_format(&buffer, formstr, va_arg(arg, void *)); + print_format(&buffer, + bufsize - (buffer - bstart), + formstr, va_arg(arg, void *)); } ptr++; } else {