From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753129AbbFHP5a (ORCPT <rfc822;w@1wt.eu>);
	Mon, 8 Jun 2015 11:57:30 -0400
Received: from smtprelay0124.hostedemail.com ([216.40.44.124]:57716 "EHLO
	smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL)
	by vger.kernel.org with ESMTP id S1751570AbbFHP5T (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 8 Jun 2015 11:57:19 -0400
X-Session-Marker: 6A6F6540706572636865732E636F6D
X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::,RULES_HIT:1:41:69:355:379:541:599:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1593:1594:1605:1730:1747:1777:1792:1801:2393:2553:2559:2562:2638:2693:2828:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3871:3872:3873:3874:4321:4605:5007:6261:6691:8660:9149:10004:10226:10848:11026:11232:11473:11658:11914:12043:12050:12296:12438:12517:12519:12555:12683:12740:13148:13230:13972:21060:21080,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0
X-HE-Tag: peace57_495906550909
X-Filterd-Recvd-Size: 14069
Message-ID: <1433779028.2742.10.camel@perches.com>
Subject: Re: [PATCH v2] staging: fbtft: fix out of bound access
From: Joe Perches <joe@perches.com>
To: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
        Noralf =?ISO-8859-1?Q?Tr=F8nnes?= <noralf@tronnes.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Date: Mon, 08 Jun 2015 08:57:08 -0700
In-Reply-To: <20150608145201.GA15145@sudip-PC>
References: <1433424892-23333-1-git-send-email-sudipm.mukherjee@gmail.com>
	 <1433450911.2658.6.camel@perches.com> <20150605045257.GA4012@sudip-PC>
	 <1433483211.2658.22.camel@perches.com> <20150608145201.GA15145@sudip-PC>
Content-Type: text/plain; charset="ISO-8859-1"
X-Mailer: Evolution 3.12.11-0ubuntu3 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 2015-06-08 at 20:22 +0530, Sudip Mukherjee wrote:
> On Thu, Jun 04, 2015 at 10:46:51PM -0700, Joe Perches wrote:
> > On Fri, 2015-06-05 at 10:22 +0530, Sudip Mukherjee wrote:
> > > On Thu, Jun 04, 2015 at 01:48:31PM -0700, Joe Perches wrote:
> > []
> <snip>
> > I looked at it a bit more and there's a macro that calls
> > write_register so there are actually many more call sites.
> > 
> > It's a bit non trivial to change the macro as all the
> > called (*write_register) functions would need changing
> > and these functions use va_list.
> > 
> > Maybe if you _really_ feel like it, but it's a bit of work.
> Hi Joe,
> I was doing this one today, and just changed write_reg8_bus8 to test.
> but when started compiling I found out another variation:
> #define write_reg(par, ...)                                              \
> 	par->fbtftops.write_register(par, NUMARGS(__VA_ARGS__), __VA_ARGS__)
> 
> and there are only 870 calls to write_reg. :(
> I was making it like void write_reg8_bus8(struct fbtft_par *par, int len, int *sbuf)
> but if i have to add an integer array to the places where write_reg is called
> it will become a big change. Any simple idea?

No, because each function will need to be changed.

Maybe something like this is a start, but it doesn't
compile properly because more functions need to be
changed and I haven't tested it.

 drivers/staging/fbtft/fbtft-bus.c  | 121 +++++++++++++++++--------------------
 drivers/staging/fbtft/fbtft-core.c |  36 +----------
 drivers/staging/fbtft/fbtft.h      |  16 ++---
 3 files changed, 68 insertions(+), 105 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-bus.c b/drivers/staging/fbtft/fbtft-bus.c
index 912c632..292564e 100644
--- a/drivers/staging/fbtft/fbtft-bus.c
+++ b/drivers/staging/fbtft/fbtft-bus.c
@@ -13,79 +13,74 @@
  *
  *****************************************************************************/
 
-#define define_fbtft_write_reg(func, type, modifier)                          \
-void func(struct fbtft_par *par, int len, ...)                                \
-{                                                                             \
-	va_list args;                                                         \
-	int i, ret;                                                           \
-	int offset = 0;                                                       \
-	type *buf = (type *)par->buf;                                         \
-									      \
-	if (unlikely(par->debug & DEBUG_WRITE_REGISTER)) {                    \
-		va_start(args, len);                                          \
-		for (i = 0; i < len; i++) {                                   \
-			buf[i] = (type)va_arg(args, unsigned int);            \
-		}                                                             \
-		va_end(args);                                                 \
-		fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par, par->info->device, type, buf, len, "%s: ", __func__);   \
-	}                                                                     \
-									      \
-	va_start(args, len);                                                  \
-									      \
-	if (par->startbyte) {                                                 \
-		*(u8 *)par->buf = par->startbyte;                             \
-		buf = (type *)(par->buf + 1);                                 \
-		offset = 1;                                                   \
-	}                                                                     \
-									      \
-	*buf = modifier((type)va_arg(args, unsigned int));                    \
-	if (par->gpio.dc != -1)                                               \
-		gpio_set_value(par->gpio.dc, 0);                              \
-	ret = par->fbtftops.write(par, par->buf, sizeof(type)+offset);        \
-	if (ret < 0) {                                                        \
-		va_end(args);                                                 \
-		dev_err(par->info->device, "%s: write() failed and returned %d\n", __func__, ret); \
-		return;                                                       \
-	}                                                                     \
-	len--;                                                                \
-									      \
-	if (par->startbyte)                                                   \
-		*(u8 *)par->buf = par->startbyte | 0x2;                       \
-									      \
-	if (len) {                                                            \
-		i = len;                                                      \
-		while (i--) {                                                 \
-			*buf++ = modifier((type)va_arg(args, unsigned int));  \
-		}                                                             \
-		if (par->gpio.dc != -1)                                       \
-			gpio_set_value(par->gpio.dc, 1);                      \
-		ret = par->fbtftops.write(par, par->buf, len * (sizeof(type)+offset)); \
-		if (ret < 0) {                                                \
-			va_end(args);                                         \
-			dev_err(par->info->device, "%s: write() failed and returned %d\n", __func__, ret); \
-			return;                                               \
-		}                                                             \
-	}                                                                     \
-	va_end(args);                                                         \
-}                                                                             \
+#define define_fbtft_write_reg(func, type, modifier)			\
+void func(struct fbtft_par *par, int len, const int *regs)		\
+{									\
+	int i, ret;							\
+	int offset = 0;							\
+	type *buf = (type *)par->buf;					\
+									\
+	if (unlikely(par->debug & DEBUG_WRITE_REGISTER)) {		\
+		for (i = 0; i < len; i++)				\
+			buf[i] = (type)regs[i];				\
+		fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,		\
+				  par->info->device, type, buf, len,	\
+				  "%s: ", __func__);			\
+	}								\
+									\
+	if (par->startbyte) {						\
+		*(u8 *)par->buf = par->startbyte;			\
+		buf = (type *)(par->buf + 1);				\
+		offset = 1;						\
+	}								\
+									\
+	*buf = modifier((type)regs[0]);					\
+	if (par->gpio.dc != -1)						\
+		gpio_set_value(par->gpio.dc, 0);			\
+	ret = par->fbtftops.write(par, par->buf, sizeof(type)+offset);	\
+	if (ret < 0) {							\
+		dev_err(par->info->device,				\
+			"%s: write() failed and returned %d\n",		\
+			__func__, ret);					\
+		return;							\
+	}								\
+	len--;								\
+									\
+	if (par->startbyte)						\
+		*(u8 *)par->buf = par->startbyte | 0x2;			\
+									\
+	if (len) {							\
+		i = len;						\
+		while (i--) {						\
+			*buf++ = modifier((type)regs[len - i]);		\
+		}							\
+		if (par->gpio.dc != -1)					\
+			gpio_set_value(par->gpio.dc, 1);		\
+		ret = par->fbtftops.write(par, par->buf,		\
+					  len * (sizeof(type)+offset)); \
+		if (ret < 0) {						\
+			dev_err(par->info->device,			\
+				"%s: write() failed and returned %d\n",	\
+				__func__, ret);				\
+			return;						\
+		}							\
+	}								\
+}									\
 EXPORT_SYMBOL(func);
 
 define_fbtft_write_reg(fbtft_write_reg8_bus8, u8, )
 define_fbtft_write_reg(fbtft_write_reg16_bus8, u16, cpu_to_be16)
 define_fbtft_write_reg(fbtft_write_reg16_bus16, u16, )
 
-void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
+void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, const int *regs)
 {
-	va_list args;
 	int i, ret;
 	int pad = 0;
 	u16 *buf = (u16 *)par->buf;
 
 	if (unlikely(par->debug & DEBUG_WRITE_REGISTER)) {
-		va_start(args, len);
 		for (i = 0; i < len; i++)
-			*(((u8 *)buf) + i) = (u8)va_arg(args, unsigned int);
-		va_end(args);
+			*(((u8 *)buf) + i) = (u8)regs[i];
 		fbtft_par_dbg_hex(DEBUG_WRITE_REGISTER, par,
 			par->info->device, u8, buf, len, "%s: ", __func__);
 	}
@@ -100,14 +95,12 @@ void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...)
 			*buf++ = 0x000;
 	}
 
-	va_start(args, len);
-	*buf++ = (u8)va_arg(args, unsigned int);
+	*buf++ = (u8)regs[len - 1];
 	i = len - 1;
 	while (i--) {
-		*buf = (u8)va_arg(args, unsigned int);
+		*buf = (u8)regs[i];
 		*buf++ |= 0x100; /* dc=1 */
 	}
-	va_end(args);
 	ret = par->fbtftops.write(par, par->buf, (len + pad) * sizeof(u16));
 	if (ret < 0) {
 		dev_err(par->info->device,
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index ce64521..0d925f5 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1102,23 +1102,7 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
 			fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
 				"init: write_register:%s\n", msg);
 
-			par->fbtftops.write_register(par, i,
-				buf[0], buf[1], buf[2], buf[3],
-				buf[4], buf[5], buf[6], buf[7],
-				buf[8], buf[9], buf[10], buf[11],
-				buf[12], buf[13], buf[14], buf[15],
-				buf[16], buf[17], buf[18], buf[19],
-				buf[20], buf[21], buf[22], buf[23],
-				buf[24], buf[25], buf[26], buf[27],
-				buf[28], buf[29], buf[30], buf[31],
-				buf[32], buf[33], buf[34], buf[35],
-				buf[36], buf[37], buf[38], buf[39],
-				buf[40], buf[41], buf[42], buf[43],
-				buf[44], buf[45], buf[46], buf[47],
-				buf[48], buf[49], buf[50], buf[51],
-				buf[52], buf[53], buf[54], buf[55],
-				buf[56], buf[57], buf[58], buf[59],
-				buf[60], buf[61], buf[62], buf[63]);
+			par->fbtftops.write_register(par, i, buf);
 		} else if (val & FBTFT_OF_INIT_DELAY) {
 			fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
 				"init: msleep(%u)\n", val & 0xFFFF);
@@ -1217,23 +1201,7 @@ int fbtft_init_display(struct fbtft_par *par)
 				}
 				buf[j++] = par->init_sequence[i++];
 			}
-			par->fbtftops.write_register(par, j,
-				buf[0], buf[1], buf[2], buf[3],
-				buf[4], buf[5], buf[6], buf[7],
-				buf[8], buf[9], buf[10], buf[11],
-				buf[12], buf[13], buf[14], buf[15],
-				buf[16], buf[17], buf[18], buf[19],
-				buf[20], buf[21], buf[22], buf[23],
-				buf[24], buf[25], buf[26], buf[27],
-				buf[28], buf[29], buf[30], buf[31],
-				buf[32], buf[33], buf[34], buf[35],
-				buf[36], buf[37], buf[38], buf[39],
-				buf[40], buf[41], buf[42], buf[43],
-				buf[44], buf[45], buf[46], buf[47],
-				buf[48], buf[49], buf[50], buf[51],
-				buf[52], buf[53], buf[54], buf[55],
-				buf[56], buf[57], buf[58], buf[59],
-				buf[60], buf[61], buf[62], buf[63]);
+			par->fbtftops.write_register(par, j, buf);
 			break;
 		case -2:
 			i++;
diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h
index 9fd98cb..09d0ce2 100644
--- a/drivers/staging/fbtft/fbtft.h
+++ b/drivers/staging/fbtft/fbtft.h
@@ -85,7 +85,7 @@ struct fbtft_ops {
 	int (*write)(struct fbtft_par *par, void *buf, size_t len);
 	int (*read)(struct fbtft_par *par, void *buf, size_t len);
 	int (*write_vmem)(struct fbtft_par *par, size_t offset, size_t len);
-	void (*write_register)(struct fbtft_par *par, int len, ...);
+	void (*write_register)(struct fbtft_par *par, int len, const int *regs);
 
 	void (*set_addr_win)(struct fbtft_par *par,
 		int xs, int ys, int xe, int ye);
@@ -258,8 +258,10 @@ struct fbtft_par {
 
 #define NUMARGS(...)  (sizeof((int[]){__VA_ARGS__})/sizeof(int))
 
-#define write_reg(par, ...)                                              \
-	par->fbtftops.write_register(par, NUMARGS(__VA_ARGS__), __VA_ARGS__)
+#define write_reg(par, ...)						\
+	par->fbtftops.write_register(par,				\
+				     NUMARGS(__VA_ARGS__),		\
+				     ((int[]){__VA_ARGS__}))
 
 /* fbtft-core.c */
 extern void fbtft_dbg_hex(const struct device *dev,
@@ -291,10 +293,10 @@ extern int fbtft_write_vmem8_bus8(struct fbtft_par *par, size_t offset, size_t l
 extern int fbtft_write_vmem16_bus16(struct fbtft_par *par, size_t offset, size_t len);
 extern int fbtft_write_vmem16_bus8(struct fbtft_par *par, size_t offset, size_t len);
 extern int fbtft_write_vmem16_bus9(struct fbtft_par *par, size_t offset, size_t len);
-extern void fbtft_write_reg8_bus8(struct fbtft_par *par, int len, ...);
-extern void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, ...);
-extern void fbtft_write_reg16_bus8(struct fbtft_par *par, int len, ...);
-extern void fbtft_write_reg16_bus16(struct fbtft_par *par, int len, ...);
+extern void fbtft_write_reg8_bus8(struct fbtft_par *par, int len, const int *regs);
+extern void fbtft_write_reg8_bus9(struct fbtft_par *par, int len, const int *regs);
+extern void fbtft_write_reg16_bus8(struct fbtft_par *par, int len, const int *regs);
+extern void fbtft_write_reg16_bus16(struct fbtft_par *par, int len, const int *regs);
 
 
 #define FBTFT_REGISTER_DRIVER(_name, _compatible, _display)                \