* [PATCH v2 0/3] Remove all strcpy() uses
@ 2021-07-24 15:14 Len Baker
2021-07-24 15:14 ` [PATCH v2 1/3] staging/fbtft: " Len Baker
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Len Baker @ 2021-07-24 15:14 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Len Baker, Andy Shevchenko, Phil Reid, Geert Uytterhoeven,
dri-devel, linux-fbdev, linux-staging, linux-kernel
strcpy() performs no bounds checking on the destination buffer. This
could result in linear overflows beyond the end of the buffer, leading
to all kinds of misbehaviors. So, this serie removes all strcpy uses
from the "staging/fbtft" subsystem.
Also, refactor the code a bit to follow the kernel coding-style and
avoid unnecessary variable initialization.
Changelog v1 -> v2
- Add two new commits to clean the code.
- Use the "%*ph" format specifier instead of strscpy() function (Geert
Uytterhoeven)
Len Baker (3):
staging/fbtft: Remove all strcpy() uses
staging/fbtft: Remove unnecessary variable initialization
staging/fbtft: Fix braces coding style
drivers/staging/fbtft/fbtft-core.c | 30 +++++++++++++-----------------
1 file changed, 13 insertions(+), 17 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses 2021-07-24 15:14 [PATCH v2 0/3] Remove all strcpy() uses Len Baker @ 2021-07-24 15:14 ` Len Baker 2021-07-24 20:21 ` Andy Shevchenko 2021-07-24 15:14 ` [PATCH v2 2/3] staging/fbtft: Remove unnecessary variable initialization Len Baker 2021-07-24 15:14 ` [PATCH v2 3/3] staging/fbtft: Fix braces coding style Len Baker 2 siblings, 1 reply; 10+ messages in thread From: Len Baker @ 2021-07-24 15:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Len Baker, Andy Shevchenko, Phil Reid, Geert Uytterhoeven, dri-devel, linux-fbdev, linux-staging, linux-kernel strcpy() performs no bounds checking on the destination buffer. This could result in linear overflows beyond the end of the buffer, leading to all kinds of misbehaviors. The safe replacement is strscpy() but in this case it is simpler to use the "%*ph" format specifier. Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/staging/fbtft/fbtft-core.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 3723269890d5..be20da3c4a5c 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -992,8 +992,6 @@ static int fbtft_init_display_from_property(struct fbtft_par *par) int fbtft_init_display(struct fbtft_par *par) { int buf[64]; - char msg[128]; - char str[16]; int i = 0; int j; @@ -1036,17 +1034,14 @@ int fbtft_init_display(struct fbtft_par *par) switch (par->init_sequence[i]) { case -1: i++; + /* make debug message */ - strcpy(msg, ""); - j = i + 1; - while (par->init_sequence[j] >= 0) { - sprintf(str, "0x%02X ", par->init_sequence[j]); - strcat(msg, str); - j++; - } + for (j = i + 1; par->init_sequence[j] >= 0; j++); + fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, - "init: write(0x%02X) %s\n", - par->init_sequence[i], msg); + "init: write(0x%02X) %*ph\n", + par->init_sequence[i], j - i - 1, + &par->init_sequence[i + 1]); /* Write */ j = 0; -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses 2021-07-24 15:14 ` [PATCH v2 1/3] staging/fbtft: " Len Baker @ 2021-07-24 20:21 ` Andy Shevchenko 2021-07-25 13:58 ` Len Baker 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2021-07-24 20:21 UTC (permalink / raw) To: Len Baker Cc: Greg Kroah-Hartman, Andy Shevchenko, Phil Reid, Geert Uytterhoeven, dri-devel, open list:FRAMEBUFFER LAYER, linux-staging, Linux Kernel Mailing List On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > strcpy() performs no bounds checking on the destination buffer. This > could result in linear overflows beyond the end of the buffer, leading > to all kinds of misbehaviors. The safe replacement is strscpy() but in > this case it is simpler to use the "%*ph" format specifier. ... > - char msg[128]; 128 / 4 = 32. So, this buffer is enough to debug print only up to 32 bytes. Hence %*ph replacement won't cut output earlier than requested. ... > + for (j = i + 1; par->init_sequence[j] >= 0; j++); Why is i + 1 initial for the j? You may rather access the 'i + 1 + j'th element in the array... ... > + par->init_sequence[i], j - i - 1, ...and get rid of the ' - i -1' part here. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses 2021-07-24 20:21 ` Andy Shevchenko @ 2021-07-25 13:58 ` Len Baker 2021-07-25 18:51 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: Len Baker @ 2021-07-25 13:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Len Baker, Greg Kroah-Hartman, Phil Reid, Geert Uytterhoeven, dri-devel, open list:FRAMEBUFFER LAYER, linux-staging, Linux Kernel Mailing List Hi, On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > > > strcpy() performs no bounds checking on the destination buffer. This > > could result in linear overflows beyond the end of the buffer, leading > > to all kinds of misbehaviors. The safe replacement is strscpy() but in > > this case it is simpler to use the "%*ph" format specifier. > > ... > > > - char msg[128]; > > 128 / 4 = 32. So, this buffer is enough to debug print only up to 32 > bytes. Hence %*ph replacement won't cut output earlier than requested. I'm sorry, but I don't understand what you are trying to explain. Moreover, with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can print 128/5 values (25 hex values). The %*ph replacement can print up to 64 bytes, so I don't see any problem here. > > ... > > > + for (j = i + 1; par->init_sequence[j] >= 0; j++); > > Why is i + 1 initial for the j? You may rather access the 'i + 1 + > j'th element in the array... > > ... > > > + par->init_sequence[i], j - i - 1, > > ...and get rid of the ' - i -1' part here. Yes, it was the first idea but I prefer this method since we save aritmethic operations. In other words, if I use what you suggest, the index for par->init_sequence is calculated as a "sum" every iteration. But if the performance is not an issue and you believe that the above is more clear, I have no problem. What do you prefer? Thanks, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses 2021-07-25 13:58 ` Len Baker @ 2021-07-25 18:51 ` Andy Shevchenko 2021-07-31 13:43 ` Len Baker 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2021-07-25 18:51 UTC (permalink / raw) To: Len Baker Cc: Greg Kroah-Hartman, Phil Reid, Geert Uytterhoeven, dri-devel, open list:FRAMEBUFFER LAYER, linux-staging, Linux Kernel Mailing List On Sun, Jul 25, 2021 at 4:59 PM Len Baker <len.baker@gmx.com> wrote: > On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: ... > > > - char msg[128]; > > > > 128 / 4 = 32. So, this buffer is enough to debug print only up to 32 > > bytes. Hence %*ph replacement won't cut output earlier than requested. > > I'm sorry, but I don't understand what you are trying to explain. Moreover, > with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can > print 128/5 values (25 hex values). > > The %*ph replacement can print up to 64 bytes, so I don't see any problem > here. Right. That's what I am trying to say and the hint here is to combine this part into a phrase in the commit message in the next version of the patch. ... > > > + for (j = i + 1; par->init_sequence[j] >= 0; j++); > > > > Why is i + 1 initial for the j? You may rather access the 'i + 1 + > > j'th element in the array... > > > > ... > > > > > + par->init_sequence[i], j - i - 1, > > > > ...and get rid of the ' - i -1' part here. > > Yes, it was the first idea but I prefer this method since we save aritmethic > operations. In other words, if I use what you suggest, the index for > par->init_sequence is calculated as a "sum" every iteration. But if the > performance is not an issue and you believe that the above is more clear, I > have no problem. What do you prefer? I prefer my variant and I believe the compilers nowadays are clever enough to understand this. Have you tried to compile and compare the real assembly? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses 2021-07-25 18:51 ` Andy Shevchenko @ 2021-07-31 13:43 ` Len Baker 0 siblings, 0 replies; 10+ messages in thread From: Len Baker @ 2021-07-31 13:43 UTC (permalink / raw) To: Andy Shevchenko Cc: Len Baker, Greg Kroah-Hartman, Phil Reid, Geert Uytterhoeven, dri-devel, open list:FRAMEBUFFER LAYER, linux-staging, Linux Kernel Mailing List On Sun, Jul 25, 2021 at 09:51:18PM +0300, Andy Shevchenko wrote: > On Sun, Jul 25, 2021 at 4:59 PM Len Baker <len.baker@gmx.com> wrote: > > On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote: > > > On Sat, Jul 24, 2021 at 7:05 PM Len Baker <len.baker@gmx.com> wrote: > > ... > > > > > - char msg[128]; > > > > > > 128 / 4 = 32. So, this buffer is enough to debug print only up to 32 > > > bytes. Hence %*ph replacement won't cut output earlier than requested. > > > > I'm sorry, but I don't understand what you are trying to explain. Moreover, > > with the "0x%02X " in the sprintf followed by the strcat, the msg buffer can > > print 128/5 values (25 hex values). > > > > The %*ph replacement can print up to 64 bytes, so I don't see any problem > > here. > > Right. That's what I am trying to say and the hint here is to combine > this part into a phrase in the commit message in the next version of > the patch. Ok, I will update the commit changelog for the next version. > > ... > > > > > + for (j = i + 1; par->init_sequence[j] >= 0; j++); > > > > > > Why is i + 1 initial for the j? You may rather access the 'i + 1 + > > > j'th element in the array... > > > > > > ... > > > > > > > + par->init_sequence[i], j - i - 1, > > > > > > ...and get rid of the ' - i -1' part here. > > > > Yes, it was the first idea but I prefer this method since we save aritmethic > > operations. In other words, if I use what you suggest, the index for > > par->init_sequence is calculated as a "sum" every iteration. But if the > > performance is not an issue and you believe that the above is more clear, I > > have no problem. What do you prefer? > > I prefer my variant and I believe the compilers nowadays are clever > enough to understand this. Ok, understood. Thanks. > Have you tried to compile and compare the real assembly? I will test it. > -- > With Best Regards, > Andy Shevchenko Regards, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] staging/fbtft: Remove unnecessary variable initialization 2021-07-24 15:14 [PATCH v2 0/3] Remove all strcpy() uses Len Baker 2021-07-24 15:14 ` [PATCH v2 1/3] staging/fbtft: " Len Baker @ 2021-07-24 15:14 ` Len Baker 2021-07-24 15:14 ` [PATCH v2 3/3] staging/fbtft: Fix braces coding style Len Baker 2 siblings, 0 replies; 10+ messages in thread From: Len Baker @ 2021-07-24 15:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Len Baker, Andy Shevchenko, Phil Reid, Geert Uytterhoeven, dri-devel, linux-fbdev, linux-staging, linux-kernel Remove the initialization of the variable "i" since it is written a few lines later. Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/staging/fbtft/fbtft-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index be20da3c4a5c..cc2bee22f7ad 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -992,7 +992,7 @@ static int fbtft_init_display_from_property(struct fbtft_par *par) int fbtft_init_display(struct fbtft_par *par) { int buf[64]; - int i = 0; + int i; int j; /* sanity check */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] staging/fbtft: Fix braces coding style 2021-07-24 15:14 [PATCH v2 0/3] Remove all strcpy() uses Len Baker 2021-07-24 15:14 ` [PATCH v2 1/3] staging/fbtft: " Len Baker 2021-07-24 15:14 ` [PATCH v2 2/3] staging/fbtft: Remove unnecessary variable initialization Len Baker @ 2021-07-24 15:14 ` Len Baker 2021-07-24 18:01 ` Geert Uytterhoeven 2 siblings, 1 reply; 10+ messages in thread From: Len Baker @ 2021-07-24 15:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Len Baker, Andy Shevchenko, Phil Reid, Geert Uytterhoeven, dri-devel, linux-fbdev, linux-staging, linux-kernel Add braces to the "for" loop and remove braces from the "if" statement. This way the kernel coding style is followed. Signed-off-by: Len Baker <len.baker@gmx.com> --- drivers/staging/fbtft/fbtft-core.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index cc2bee22f7ad..d87792649efe 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -1003,9 +1003,11 @@ int fbtft_init_display(struct fbtft_par *par) } /* make sure stop marker exists */ - for (i = 0; i < FBTFT_MAX_INIT_SEQUENCE; i++) + for (i = 0; i < FBTFT_MAX_INIT_SEQUENCE; i++) { if (par->init_sequence[i] == -3) break; + } + if (i == FBTFT_MAX_INIT_SEQUENCE) { dev_err(par->info->device, "missing stop marker at end of init sequence\n"); @@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par) i = 0; while (i < FBTFT_MAX_INIT_SEQUENCE) { - if (par->init_sequence[i] == -3) { - /* done */ - return 0; - } + if (par->init_sequence[i] == -3) + return 0; /* done */ + if (par->init_sequence[i] >= 0) { dev_err(par->info->device, "missing delimiter at position %d\n", i); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] staging/fbtft: Fix braces coding style 2021-07-24 15:14 ` [PATCH v2 3/3] staging/fbtft: Fix braces coding style Len Baker @ 2021-07-24 18:01 ` Geert Uytterhoeven 2021-07-25 14:02 ` Len Baker 0 siblings, 1 reply; 10+ messages in thread From: Geert Uytterhoeven @ 2021-07-24 18:01 UTC (permalink / raw) To: Len Baker Cc: Greg Kroah-Hartman, Andy Shevchenko, Phil Reid, DRI Development, Linux Fbdev development list, linux-staging, Linux Kernel Mailing List Hi Len, On Sat, Jul 24, 2021 at 7:44 PM Len Baker <len.baker@gmx.com> wrote: > Add braces to the "for" loop and remove braces from the "if" statement. > This way the kernel coding style is followed. > > Signed-off-by: Len Baker <len.baker@gmx.com> Thanks for your patch! > --- a/drivers/staging/fbtft/fbtft-core.c > +++ b/drivers/staging/fbtft/fbtft-core.c > @@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par) > > i = 0; > while (i < FBTFT_MAX_INIT_SEQUENCE) { > - if (par->init_sequence[i] == -3) { > - /* done */ > - return 0; > - } These braces should not be removed, due to the presence of the comment. > + if (par->init_sequence[i] == -3) > + return 0; /* done */ > + > if (par->init_sequence[i] >= 0) { > dev_err(par->info->device, > "missing delimiter at position %d\n", i); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] staging/fbtft: Fix braces coding style 2021-07-24 18:01 ` Geert Uytterhoeven @ 2021-07-25 14:02 ` Len Baker 0 siblings, 0 replies; 10+ messages in thread From: Len Baker @ 2021-07-25 14:02 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Len Baker, Greg Kroah-Hartman, Andy Shevchenko, Phil Reid, DRI Development, Linux Fbdev development list, linux-staging, Linux Kernel Mailing List Hi, On Sat, Jul 24, 2021 at 08:01:53PM +0200, Geert Uytterhoeven wrote: > Hi Len, > > On Sat, Jul 24, 2021 at 7:44 PM Len Baker <len.baker@gmx.com> wrote: > > Add braces to the "for" loop and remove braces from the "if" statement. > > This way the kernel coding style is followed. > > > > Signed-off-by: Len Baker <len.baker@gmx.com> > > Thanks for your patch! > > > --- a/drivers/staging/fbtft/fbtft-core.c > > +++ b/drivers/staging/fbtft/fbtft-core.c > > > @@ -1016,10 +1018,9 @@ int fbtft_init_display(struct fbtft_par *par) > > > > i = 0; > > while (i < FBTFT_MAX_INIT_SEQUENCE) { > > - if (par->init_sequence[i] == -3) { > > - /* done */ > > - return 0; > > - } > > These braces should not be removed, due to the presence of > the comment. Ok, I leave it as is. Thanks, Len ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-07-31 13:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-24 15:14 [PATCH v2 0/3] Remove all strcpy() uses Len Baker 2021-07-24 15:14 ` [PATCH v2 1/3] staging/fbtft: " Len Baker 2021-07-24 20:21 ` Andy Shevchenko 2021-07-25 13:58 ` Len Baker 2021-07-25 18:51 ` Andy Shevchenko 2021-07-31 13:43 ` Len Baker 2021-07-24 15:14 ` [PATCH v2 2/3] staging/fbtft: Remove unnecessary variable initialization Len Baker 2021-07-24 15:14 ` [PATCH v2 3/3] staging/fbtft: Fix braces coding style Len Baker 2021-07-24 18:01 ` Geert Uytterhoeven 2021-07-25 14:02 ` Len Baker
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).