From: Len Baker <hidden> Date: 2021-07-24 15:14:49
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
From: Len Baker <hidden> Date: 2021-07-24 16:05:02
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 <redacted>
---
drivers/staging/fbtft/fbtft-core.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
From: Len Baker <hidden> Date: 2021-07-24 16:55:05
Remove the initialization of the variable "i" since it is written a few
lines later.
Signed-off-by: Len Baker <redacted>
---
drivers/staging/fbtft/fbtft-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Len Baker <hidden> Date: 2021-07-24 17:45:02
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 <redacted>
---
drivers/staging/fbtft/fbtft-core.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
@@ -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 */-return0;-}+if(par->init_sequence[i]==-3)+return0;/* done */+if(par->init_sequence[i]>=0){dev_err(par->info->device,"missing delimiter at position %d\n",i);--
Hi Len,
On Sat, Jul 24, 2021 at 7:44 PM Len Baker [off-list ref] 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 <redacted>
@@ -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
From: Andy Shevchenko <hidden> Date: 2021-07-24 20:21:44
On Sat, Jul 24, 2021 at 7:05 PM Len Baker [off-list ref] 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
From: Len Baker <hidden> Date: 2021-07-25 13:59:18
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 [off-list ref] wrote:
quoted
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.
...
quoted
- 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.
...
quoted
+ 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...
...
quoted
+ 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
From: Len Baker <hidden> Date: 2021-07-25 14:02:29
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 [off-list ref] wrote:
quoted
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 <redacted>
From: Andy Shevchenko <hidden> Date: 2021-07-25 18:51:58
On Sun, Jul 25, 2021 at 4:59 PM Len Baker [off-list ref] wrote:
On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote:
quoted
On Sat, Jul 24, 2021 at 7:05 PM Len Baker [off-list ref] wrote:
...
quoted
quoted
- 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.
...
quoted
quoted
+ 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...
...
quoted
+ 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
From: Len Baker <hidden> Date: 2021-07-31 13:43:56
On Sun, Jul 25, 2021 at 09:51:18PM +0300, Andy Shevchenko wrote:
On Sun, Jul 25, 2021 at 4:59 PM Len Baker [off-list ref] wrote:
quoted
On Sat, Jul 24, 2021 at 11:21:04PM +0300, Andy Shevchenko wrote:
quoted
On Sat, Jul 24, 2021 at 7:05 PM Len Baker [off-list ref] wrote:
...
quoted
quoted
quoted
- 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.
...
quoted
quoted
quoted
+ 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...
...
quoted
+ 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?