* [PATCH v2] staging: fbtft: fix out of bound access
@ 2015-06-04 13:34 Sudip Mukherjee
2015-06-04 20:48 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Sudip Mukherjee @ 2015-06-04 13:34 UTC (permalink / raw)
To: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman
Cc: devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: fbtft: fix out of bound access
2015-06-04 13:34 [PATCH v2] staging: fbtft: fix out of bound access Sudip Mukherjee
@ 2015-06-04 20:48 ` Joe Perches
2015-06-05 4:52 ` Sudip Mukherjee
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2015-06-04 20:48 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman, devel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: fbtft: fix out of bound access
2015-06-04 20:48 ` Joe Perches
@ 2015-06-05 4:52 ` Sudip Mukherjee
2015-06-05 5:46 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Sudip Mukherjee @ 2015-06-05 4:52 UTC (permalink / raw)
To: Joe Perches
Cc: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman, devel,
linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: fbtft: fix out of bound access
2015-06-05 4:52 ` Sudip Mukherjee
@ 2015-06-05 5:46 ` Joe Perches
2015-06-08 14:52 ` Sudip Mukherjee
0 siblings, 1 reply; 6+ messages in thread
From: Joe Perches @ 2015-06-05 5:46 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman, devel,
linux-kernel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: fbtft: fix out of bound access
2015-06-05 5:46 ` Joe Perches
@ 2015-06-08 14:52 ` Sudip Mukherjee
2015-06-08 15:57 ` Joe Perches
0 siblings, 1 reply; 6+ messages in thread
From: Sudip Mukherjee @ 2015-06-08 14:52 UTC (permalink / raw)
To: Joe Perches
Cc: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman, devel,
linux-kernel
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] staging: fbtft: fix out of bound access
2015-06-08 14:52 ` Sudip Mukherjee
@ 2015-06-08 15:57 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2015-06-08 15:57 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Thomas Petazzoni, Noralf Trønnes, Greg Kroah-Hartman, devel,
linux-kernel
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) \
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-06-08 15:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-04 13:34 [PATCH v2] staging: fbtft: fix out of bound access Sudip Mukherjee
2015-06-04 20:48 ` Joe Perches
2015-06-05 4:52 ` Sudip Mukherjee
2015-06-05 5:46 ` Joe Perches
2015-06-08 14:52 ` Sudip Mukherjee
2015-06-08 15:57 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).