[PATCH v2] staging: fbtft: fix out of bound access

[PATCH v2] staging: fbtft: fix out of bound access

[Sudip Mukherjee]

str was 16 bytes but was mentioned as 128 in snprintf.
again msg is 128 bytes but not sufficient to hold the complete debug
message of register values.
Now removed the use of str, msg and print the register values from the
loop.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v2: removed the use of msg and str.

 drivers/staging/fbtft/fbtft-core.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
index ce64521..c1502c3 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1067,8 +1067,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
 	const __be32 *p;
 	u32 val;
 	int buf[64], i, j;
-	char msg[128];
-	char str[16];
 
 	fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "%s()\n", __func__);
 
@@ -1094,13 +1092,11 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
 				p = of_prop_next_u32(prop, p, &val);
 			}
 			/* make debug message */
-			msg[0] = '\0';
-			for (j = 0; j < i; j++) {
-				snprintf(str, 128, " %02X", buf[j]);
-				strcat(msg, str);
-			}
 			fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
 				"init: write_register:%s\n", msg);
+			for (j = 0; j < i; j++)
+				fbtft_par_dbg(DEBUG_INIT_DISPLAY, par,
+					      "buf[%d] = %02X\n", j, buf[j]);
 
 			par->fbtftops.write_register(par, i,
 				buf[0], buf[1], buf[2], buf[3],
-- 
1.8.1.2

Re: [PATCH v2] staging: fbtft: fix out of bound access

[Joe Perches]

On Thu, 2015-06-04 at 19:04 +0530, Sudip Mukherjee wrote:
> str was 16 bytes but was mentioned as 128 in snprintf.
> again msg is 128 bytes but not sufficient to hold the complete debug
> message of register values.
> Now removed the use of str, msg and print the register values from the
> loop.
[]
> v2: removed the use of msg and str.

It's nice to cc the people that give you suggestions when
you send a new version of a patch.

> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
[]
> @@ -1067,8 +1067,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
>  	const __be32 *p;
>  	u32 val;
>  	int buf[64], i, j;
[]
>  			par->fbtftops.write_register(par, i,
>  				buf[0], buf[1], buf[2], buf[3],

It seems there are only 2 callers of (*write_register)()
and the arguments are always an in-order array int[64]

Maybe it'd be nicer to change the prototypes of the
write_register functions to take a const int * 
instead of pushing 64 ints on the stack.

Re: [PATCH v2] staging: fbtft: fix out of bound access

[Sudip Mukherjee]

On Thu, Jun 04, 2015 at 01:48:31PM -0700, Joe Perches wrote:
> On Thu, 2015-06-04 at 19:04 +0530, Sudip Mukherjee wrote:
> > str was 16 bytes but was mentioned as 128 in snprintf.
> > again msg is 128 bytes but not sufficient to hold the complete debug
> > message of register values.
> > Now removed the use of str, msg and print the register values from the
> > loop.
> []
> > v2: removed the use of msg and str.
> 
> It's nice to cc the people that give you suggestions when
> you send a new version of a patch.
oops, i thought but apparently i forgot while sending.
actuially I was going through one of your advise you gave long ago
(https://lkml.org/lkml/2014/10/11/85) , and I was trying to find
out the effects of that on the way i have done the v2 and I changed
it again to arrive at the posted patch. While doing all these, ccing
you just slipped out of my mind. sorry again.
> 
> > diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> []
> > @@ -1067,8 +1067,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
> >  	const __be32 *p;
> >  	u32 val;
> >  	int buf[64], i, j;
> []
> >  			par->fbtftops.write_register(par, i,
> >  				buf[0], buf[1], buf[2], buf[3],
> 
> It seems there are only 2 callers of (*write_register)()
> and the arguments are always an in-order array int[64]
> 
> Maybe it'd be nicer to change the prototypes of the
> write_register functions to take a const int * 
> instead of pushing 64 ints on the stack.
yes, I will send it as a separate patch as that is another change.

regards
sudip

> 

Re: [PATCH v2] staging: fbtft: fix out of bound access

[Joe Perches]

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:
[]
> ccing you just slipped out of my mind.

No worries.

> > > diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c
> > []
> > > @@ -1067,8 +1067,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par)
> > >  	const __be32 *p;
> > >  	u32 val;
> > >  	int buf[64], i, j;
> > []
> > >  			par->fbtftops.write_register(par, i,
> > >  				buf[0], buf[1], buf[2], buf[3],
> > 
> > It seems there are only 2 callers of (*write_register)()
> > and the arguments are always an in-order array int[64]
> > 
> > Maybe it'd be nicer to change the prototypes of the
> > write_register functions to take a const int * 
> > instead of pushing 64 ints on the stack.
> yes, I will send it as a separate patch as that is another change.

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.

Re: [PATCH v2] staging: fbtft: fix out of bound access

[Sudip Mukherjee]

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?

regards
sudip 
> 

Re: [PATCH v2] staging: fbtft: fix out of bound access

[Joe Perches]

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