Re: [PATCH] staging/fbtft: Remove all strcpy() uses
Hi,
On Sun, Jul 18, 2021 at 10:42:42PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 18, 2021 at 4:43 PM Len Baker 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 add NULL to the first position since we want
> > to empty the string.
>
> > This is a previous step in the path to remove the strcpy() function.
>
> Any document behind this (something to read on the site(s) more or
> less affiliated with what is going to happen in the kernel) to read
> background?
This is a task of the KSPP (kernel self protection project) [1]
[1] https://github.com/KSPP/linux/issues/88
>
> ...
>
> > case -1:
> > i++;
> > /* make debug message */
> > - strcpy(msg, "");
> > + msg[0] = 0;
>
> Strictly speaking it should be '\0'.
Ok, understood.
>
> > j = i + 1;
> > while (par->init_sequence[j] >= 0) {
> > sprintf(str, "0x%02X ",
> > par->init_sequence[j]);
>
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks for the feedback,
Len
Re: [PATCH] staging/fbtft: Remove all strcpy() uses
On Mon, Jul 19, 2021 at 09:53:29AM +0200, Geert Uytterhoeven wrote:
> On Sun, Jul 18, 2021 at 9:43 PM Andy Shevchenko
> wrote:
> > On Sun, Jul 18, 2021 at 4:43 PM Len Baker 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 add NULL to the first position since we want
>
> "NULL" is a pointer value, "NUL" is the character with value zero.
Ok, understood. Thanks.
>
> > > to empty the string.
> >
> > > This is a previous step in the path to remove the strcpy() function.
> >
> > Any document behind this (something to read on the site(s) more or
> > less affiliated with what is going to happen in the kernel) to read
> > background?
> >
> > ...
> >
> > > case -1:
> > > i++;
> > > /* make debug message */
> > > - strcpy(msg, "");
>
> While this strcpy() is provably safe at compile-time, and will probably
> be replaced by an assignment to zero by the compiler...
>
> > > + msg[0] = 0;
> >
> > Strictly speaking it should be '\0'.
> >
> > > j = i + 1;
> > > while (par->init_sequence[j] >= 0) {
> > > sprintf(str, "0x%02X ",
> > > par->init_sequence[j]);
>
> ... the real danger is the
>
> strcat(msg, str);
>
> on the next line.
> Fortunately this whole debug printing block (including the strcpy)
> can (and should) be rewritten to just use "%*ph".
Ok, I will work on it and I will send a v2 for review. Thanks for the
feedback.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@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
Regards,
Len
[PATCH v2 0/3] Remove all strcpy() uses
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
[PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
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
---
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
[PATCH v2 2/3] staging/fbtft: Remove unnecessary variable initialization
Remove the initialization of the variable "i" since it is written a few
lines later.
Signed-off-by: Len Baker
---
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
[PATCH v2 3/3] staging/fbtft: Fix braces coding style
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
---
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
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
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 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
Re: [PATCH v2 3/3] staging/fbtft: Fix braces coding style
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 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
>
> 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
Re: [PATCH v2 1/3] staging/fbtft: Remove all strcpy() uses
On Sun, Jul 25, 2021 at 09:51:18PM +0300, Andy Shevchenko wrote: > On Sun, Jul 25, 2021 at 4:59 PM Len Baker 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 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
[PATCH v3 0/3] Remove all strcpy() uses
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) Changelog v2 -> v3 - Change the initialization of the "j" variable in the "for" loop and update the code accordingly (Andy Shevchenko). - Improve the commit message to inform that the "%*ph" replacement won't cut output earlier than requested (Andy Shevchenko). - Don't remove the braces in the "if" statement due to the presence of the comment (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 | 23 ++- 1 file changed, 10 insertions(+), 13 deletions(-) -- 2.25.1
[PATCH v3 1/3] staging/fbtft: Remove all strcpy() uses
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.
Moreover, with the "0x%02X " in the sprintf followed by the strcat, the
msg buffer (now removed) can print 128/5 values (25 hex values). So, the
"%*ph" replacement won't cut output earlier than requested since this
format specifier can print up to 64 bytes.
Signed-off-by: Len Baker
---
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..e6286043bff7 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 = 0; par->init_sequence[i + 1 + 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,
+ &par->init_sequence[i + 1]);
/* Write */
j = 0;
--
2.25.1
[PATCH v3 2/3] staging/fbtft: Remove unnecessary variable initialization
Remove the initialization of the variable "i" since it is written a few
lines later.
Signed-off-by: Len Baker
---
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 e6286043bff7..ed896049118c 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
[PATCH v3 3/3] staging/fbtft: Fix braces coding style
Add braces to the "for" loop. This way, the kernel coding style is
followed.
Signed-off-by: Len Baker
---
drivers/staging/fbtft/fbtft-core.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c
b/drivers/staging/fbtft/fbtft-core.c
index ed896049118c..ed992ca605eb 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");
--
2.25.1
Re: [PATCH v3 0/3] Remove all strcpy() uses
Hi Andy, On Sun, Aug 01, 2021 at 02:40:40PM +0300, Andy Shevchenko wrote: > On Sun, Aug 1, 2021 at 11:53 AM Len Baker 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. 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. > > I don't see patch 3 (even on lore.kernel.org). Due to my email provider restrictions (number of emails per hour), I need to send an email every x time. Regards, Len
[PATCH] drm/amd/display: Fix identical code for different branches
The ternary expression:
vrr->state == VRR_STATE_ACTIVE_VARIABLE ? max_refresh : max_refresh;
has identical then and else expressions. So, simplify the code.
Addresses-Coverity-ID: 1471122 ("Identical code for different branches")
Fixes: 9bc4162665827 ("drm/amd/display: Implement VSIF V3 extended refresh rate
feature")
Signed-off-by: Len Baker
---
drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index 3f4f44b44e6a..54374c7d309b 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -613,9 +613,8 @@ static void build_vrr_infopacket_data_v3(const struct
mod_vrr_params *vrr,
(vrr->state == VRR_STATE_INACTIVE) ? min_refresh :
max_refresh; // Non-fs case, program nominal range
- max_programmed = (vrr->state == VRR_STATE_ACTIVE_FIXED) ? fixed_refresh
:
- (vrr->state == VRR_STATE_ACTIVE_VARIABLE) ? max_refresh
:
- max_refresh;// Non-fs case, program nominal range
+ max_programmed = (vrr->state == VRR_STATE_ACTIVE_FIXED) ?
+ fixed_refresh : max_refresh;
/* PB7 = FreeSync Minimum refresh rate (Hz) */
infopacket->sb[7] = min_programmed & 0xFF;
--
2.25.1
[PATCH] drm/amd/display: Fix identical code for different branches
The branches of the "if" statement are the same. So remove the
unnecessary if and goto statements.
Addresses-Coverity-ID: 1456916 ("Identical code for different branches")
Fixes: 4c283fdac08ab ("drm/amd/display: Add HDCP module")
Signed-off-by: Len Baker
---
drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
index de872e7958b0..d0c565567102 100644
--- a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
+++ b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
@@ -305,10 +305,8 @@ static enum mod_hdcp_status wait_for_ready(struct mod_hdcp
*hdcp,
hdcp, "bcaps_read"))
goto out;
}
- if (!mod_hdcp_execute_and_set(check_ksv_ready,
- &input->ready_check, &status,
- hdcp, "ready_check"))
- goto out;
+ mod_hdcp_execute_and_set(check_ksv_ready, &input->ready_check, &status,
+hdcp, "ready_check");
out:
return status;
}
--
2.25.1
Re: [PATCH] drm/amd/display: Fix identical code for different branches
On Sun, Jul 11, 2021 at 10:45:48AM -0700, Joe Perches wrote:
> On Sun, 2021-07-11 at 19:24 +0200, Len Baker wrote:
> > The branches of the "if" statement are the same. So remove the
> > unnecessary if and goto statements.
> >
> > Addresses-Coverity-ID: 1456916 ("Identical code for different branches")
> > Fixes: 4c283fdac08ab ("drm/amd/display: Add HDCP module")
> > Signed-off-by: Len Baker
>
> I'm not a big fan of this type of change.
>
> It's currently the same style used for six tests in this function
> and changing this last one would just make it harder to see the
> code blocks as consistent.
>
> I doubt any reasonable compiler would produce different objects.
Ok, thanks for the review. I leave it as is.
> > diff --git a/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
> > b/drivers/gpu/drm/amd/display/modules/hdcp/hdcp1_execution.c
> []
> > @@ -305,10 +305,8 @@ static enum mod_hdcp_status wait_for_ready(struct
> > mod_hdcp *hdcp,
> > hdcp, "bcaps_read"))
> > goto out;
> > }
> > - if (!mod_hdcp_execute_and_set(check_ksv_ready,
> > - &input->ready_check, &status,
> > - hdcp, "ready_check"))
> > - goto out;
> > + mod_hdcp_execute_and_set(check_ksv_ready, &input->ready_check, &status,
> > +hdcp, "ready_check");
> > out:
> > return status;
> > }
> > --
> > 2.25.1
> >
Thanks,
Len
[PATCH] staging/fbtft: Remove all strcpy() uses
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 add NULL to the first position since we want
to empty the string.
This is a previous step in the path to remove the strcpy() function.
Signed-off-by: Len Baker
---
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 3723269890d5..b8791806cb20 100644
--- a/drivers/staging/fbtft/fbtft-core.c
+++ b/drivers/staging/fbtft/fbtft-core.c
@@ -1037,7 +1037,7 @@ int fbtft_init_display(struct fbtft_par *par)
case -1:
i++;
/* make debug message */
- strcpy(msg, "");
+ msg[0] = 0;
j = i + 1;
while (par->init_sequence[j] >= 0) {
sprintf(str, "0x%02X ", par->init_sequence[j]);
--
2.25.1
[PATCH] drm/radeon: Prefer kcalloc over open coded arithmetic
As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.
So, refactor the code a bit to use the purpose specific kcalloc()
function instead of the calculated size argument in the kzalloc()
function.
[1]
https://www.kernel.org/doc/html/v5.14/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Signed-off-by: Len Baker
---
drivers/gpu/drm/radeon/r600_dpm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_dpm.c
b/drivers/gpu/drm/radeon/r600_dpm.c
index 35b77c944701..fd4226b99862 100644
--- a/drivers/gpu/drm/radeon/r600_dpm.c
+++ b/drivers/gpu/drm/radeon/r600_dpm.c
@@ -820,12 +820,12 @@ union fan_info {
static int r600_parse_clk_voltage_dep_table(struct
radeon_clock_voltage_dependency_table *radeon_table,
ATOM_PPLIB_Clock_Voltage_Dependency_Table *atom_table)
{
- u32 size = atom_table->ucNumEntries *
- sizeof(struct radeon_clock_voltage_dependency_entry);
int i;
ATOM_PPLIB_Clock_Voltage_Dependency_Record *entry;
- radeon_table->entries = kzalloc(size, GFP_KERNEL);
+ radeon_table->entries = kcalloc(atom_table->ucNumEntries,
+ sizeof(struct
radeon_clock_voltage_dependency_entry),
+ GFP_KERNEL);
if (!radeon_table->entries)
return -ENOMEM;
--
2.25.1
[PATCH] net: mana: Prefer struct_size over open coded arithmetic
As noted in the "Deprecated Interfaces, Language Features, Attributes, and Conventions" documentation [1], size calculations (especially multiplication) should not be performed in memory allocator (or similar) function arguments due to the risk of them overflowing. This could lead to values wrapping around and a smaller allocation being made than the caller was expecting. Using those allocations could lead to linear overflows of heap memory and other misbehaviors. So, use the struct_size() helper to do the arithmetic instead of the argument "size + count * size" in the kzalloc() function. [1] https://www.kernel.org/doc/html/v5.14/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments Signed-off-by: Len Baker --- drivers/net/ethernet/microsoft/mana/hw_channel.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c index 1a923fd0..0efdc6c3c32a 100644 --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c @@ -398,9 +398,7 @@ static int mana_hwc_alloc_dma_buf(struct hw_channel_context *hwc, u16 q_depth, int err; u16 i; - dma_buf = kzalloc(sizeof(*dma_buf) + - q_depth * sizeof(struct hwc_work_request), - GFP_KERNEL); + dma_buf = kzalloc(struct_size(dma_buf, reqs, q_depth), GFP_KERNEL); if (!dma_buf) return -ENOMEM; -- 2.25.1
Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic
Hi, On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > As noted in the "Deprecated Interfaces, Language Features, Attributes, > and Conventions" documentation [1], size calculations (especially > multiplication) should not be performed in memory allocator (or similar) > function arguments due to the risk of them overflowing. This could lead > to values wrapping around and a smaller allocation being made than the > caller was expecting. Using those allocations could lead to linear > overflows of heap memory and other misbehaviors. > > In this case these are not actually dynamic sizes: all the operands > involved in the calculation are constant values. However it is better to > refactor them anyway, just to keep the open-coded math idiom out of > code. > > So, add at the end of the struct i915_syncmap a union with two flexible > array members (these arrays share the same memory layout). This is > possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > struct_size() helper to do the arithmetic instead of the argument > "size + count * size" in the kmalloc and kzalloc() functions. > > Also, take the opportunity to refactor the __sync_seqno and __sync_child > making them more readable. > > This code was detected with the help of Coccinelle and audited and fixed > manually. > > [1] > https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > Signed-off-by: Len Baker > --- > drivers/gpu/drm/i915/i915_syncmap.c | 12 > 1 file changed, 8 insertions(+), 4 deletions(-) I received a mail telling that this patch doesn't build: == Series Details == Series: drm/i915: Prefer struct_size over open coded arithmetic URL : https://patchwork.freedesktop.org/series/95408/ State : failure But it builds without error against linux-next (tag next-20211001). Against which tree and branch do I need to build? Regards, Len
Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic
Hi Daniel and Jani, On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote: > On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: > > On Mon, 11 Oct 2021, Len Baker wrote: > > > Hi, > > > > > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > > >> and Conventions" documentation [1], size calculations (especially > > >> multiplication) should not be performed in memory allocator (or similar) > > >> function arguments due to the risk of them overflowing. This could lead > > >> to values wrapping around and a smaller allocation being made than the > > >> caller was expecting. Using those allocations could lead to linear > > >> overflows of heap memory and other misbehaviors. > > >> > > >> In this case these are not actually dynamic sizes: all the operands > > >> involved in the calculation are constant values. However it is better to > > >> refactor them anyway, just to keep the open-coded math idiom out of > > >> code. > > >> > > >> So, add at the end of the struct i915_syncmap a union with two flexible > > >> array members (these arrays share the same memory layout). This is > > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > > >> struct_size() helper to do the arithmetic instead of the argument > > >> "size + count * size" in the kmalloc and kzalloc() functions. > > >> > > >> Also, take the opportunity to refactor the __sync_seqno and __sync_child > > >> making them more readable. > > >> > > >> This code was detected with the help of Coccinelle and audited and fixed > > >> manually. > > >> > > >> [1] > > >> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > >> > > >> Signed-off-by: Len Baker > > >> --- > > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 > > >> 1 file changed, 8 insertions(+), 4 deletions(-) > > > > > > I received a mail telling that this patch doesn't build: > > > > > > == Series Details == > > > > > > Series: drm/i915: Prefer struct_size over open coded arithmetic > > > URL : https://patchwork.freedesktop.org/series/95408/ > > > State : failure > > > > > > But it builds without error against linux-next (tag next-20211001). > > > Against > > > which tree and branch do I need to build? > > > > drm-tip [1]. It's a sort of linux-next for graphics. I think there are > > still some branches that don't feed to linux-next. > > Yeah we need to get gt-next in linux-next asap. Joonas promised to send > out his patch to make that happen in dim. > -Daniel Is there a possibility that you give an "Acked-by" tag? And then this patch goes to the mainline through the Kees' tree or Gustavo's tree? Or is it better to wait for drm-tip to update? Regards, Len > > > > > BR, > > Jani. > > > > > > [1] https://cgit.freedesktop.org/drm/drm-tip > > > > > > > > > > Regards, > > > Len > > > > -- > > Jani Nikula, Intel Open Source Graphics Center > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic
Hi Jani, On Mon, Oct 18, 2021 at 01:00:01PM +0300, Jani Nikula wrote: > On Sat, 16 Oct 2021, Len Baker wrote: > > Hi Daniel and Jani, > > > > On Wed, Oct 13, 2021 at 01:51:30PM +0200, Daniel Vetter wrote: > >> On Wed, Oct 13, 2021 at 02:24:05PM +0300, Jani Nikula wrote: > >> > On Mon, 11 Oct 2021, Len Baker wrote: > >> > > Hi, > >> > > > >> > > On Sun, Oct 03, 2021 at 12:42:58PM +0200, Len Baker wrote: > >> > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > >> > >> and Conventions" documentation [1], size calculations (especially > >> > >> multiplication) should not be performed in memory allocator (or > >> > >> similar) > >> > >> function arguments due to the risk of them overflowing. This could > >> > >> lead > >> > >> to values wrapping around and a smaller allocation being made than the > >> > >> caller was expecting. Using those allocations could lead to linear > >> > >> overflows of heap memory and other misbehaviors. > >> > >> > >> > >> In this case these are not actually dynamic sizes: all the operands > >> > >> involved in the calculation are constant values. However it is better > >> > >> to > >> > >> refactor them anyway, just to keep the open-coded math idiom out of > >> > >> code. > >> > >> > >> > >> So, add at the end of the struct i915_syncmap a union with two > >> > >> flexible > >> > >> array members (these arrays share the same memory layout). This is > >> > >> possible using the new DECLARE_FLEX_ARRAY macro. And then, use the > >> > >> struct_size() helper to do the arithmetic instead of the argument > >> > >> "size + count * size" in the kmalloc and kzalloc() functions. > >> > >> > >> > >> Also, take the opportunity to refactor the __sync_seqno and > >> > >> __sync_child > >> > >> making them more readable. > >> > >> > >> > >> This code was detected with the help of Coccinelle and audited and > >> > >> fixed > >> > >> manually. > >> > >> > >> > >> [1] > >> > >> https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > >> > >> > >> > >> Signed-off-by: Len Baker > >> > >> --- > >> > >> drivers/gpu/drm/i915/i915_syncmap.c | 12 > >> > >> 1 file changed, 8 insertions(+), 4 deletions(-) > >> > > > >> > > I received a mail telling that this patch doesn't build: > >> > > > >> > > == Series Details == > >> > > > >> > > Series: drm/i915: Prefer struct_size over open coded arithmetic > >> > > URL : https://patchwork.freedesktop.org/series/95408/ > >> > > State : failure > >> > > > >> > > But it builds without error against linux-next (tag next-20211001). > >> > > Against > >> > > which tree and branch do I need to build? > >> > > >> > drm-tip [1]. It's a sort of linux-next for graphics. I think there are > >> > still some branches that don't feed to linux-next. > >> > >> Yeah we need to get gt-next in linux-next asap. Joonas promised to send > >> out his patch to make that happen in dim. > >> -Daniel > > > > Is there a possibility that you give an "Acked-by" tag? And then this patch > > goes to the mainline through the Kees' tree or Gustavo's tree? > > If this does not apply to drm-intel-gt-next (or drm-tip), applying it to > some other branch will just cause unnecessary conflicts later on. It's > unnecessary extra work. It's not an urgent fix or anything, there is no > reason to do that. So that's a NAK. Ok. Understood. > > Or is it better to wait for drm-tip to update? > > drm-tip is up to date, it's just that one of the branches that feed to > it is (was?) not feeding to linux-next. Sorry, but I'm missing something here. In linux-next this is the commit history of include/linux/stddef.h file: 3080ea5553cc stddef: Introduce DECLARE_FLEX_ARRAY() helper 50d7bd38c3aa stddef: Introduce struct_group() helper macro e7f18c22e6be stddef: Fix kerndoc for sizeof_field() and offsetofend() 4229a470175b stddef.h: Introduce sizeof_field() ... But in drm-tip this is the commit history: 4229a470175b stddef.h: Introduce sizeof_field() ... For this patch the DECLARE_FLEX_ARRAY() helper is needed. But the build fails due to the last tree commits for stddef.h file are not present. So, if I understand correctly, drm-tip is not up to date with linux-next. Regards, Len > > If you're contributing to drm, please consider basing your patches on > top of drm-tip. > > > BR, > Jani. > > > > > > Regards, > > Len > > > >> > >> > > >> > BR, > >> > Jani. > >> > > >> > > >> > [1] https://cgit.freedesktop.org/drm/drm-tip > >> > > >> > > >> > > > >> > > Regards, > >> > > Len > >> > > >> > -- > >> > Jani Nikula, Intel Open Source Graphics Center > >> > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> http://blog.ffwll.ch > > -- > Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] net: mana: Prefer struct_size over open coded arithmetic
Hi, On Sat, Sep 11, 2021 at 12:28:18PM +0200, Len Baker wrote: > As noted in the "Deprecated Interfaces, Language Features, Attributes, > and Conventions" documentation [1], size calculations (especially > multiplication) should not be performed in memory allocator (or similar) > function arguments due to the risk of them overflowing. This could lead > to values wrapping around and a smaller allocation being made than the > caller was expecting. Using those allocations could lead to linear > overflows of heap memory and other misbehaviors. > > So, use the struct_size() helper to do the arithmetic instead of the > argument "size + count * size" in the kzalloc() function. > > [1] > https://www.kernel.org/doc/html/v5.14/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > Signed-off-by: Len Baker > --- > drivers/net/ethernet/microsoft/mana/hw_channel.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index 1a923fd0..0efdc6c3c32a 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -398,9 +398,7 @@ static int mana_hwc_alloc_dma_buf(struct > hw_channel_context *hwc, u16 q_depth, > int err; > u16 i; > > - dma_buf = kzalloc(sizeof(*dma_buf) + > - q_depth * sizeof(struct hwc_work_request), > - GFP_KERNEL); > + dma_buf = kzalloc(struct_size(dma_buf, reqs, q_depth), GFP_KERNEL); > if (!dma_buf) > return -ENOMEM; > > -- > 2.25.1 > I have received a email from the linux-media subsystem telling that this patch is not applicable. The email is the following: Hello, The following patch (submitted by you) has been updated in Patchwork: * linux-media: net: mana: Prefer struct_size over open coded arithmetic - http://patchwork.linuxtv.org/project/linux-media/patch/20210911102818.3804-1-len.ba...@gmx.com/ - for: Linux Media kernel patches was: New now: Not Applicable This email is a notification only - you do not need to respond. The question is: Why it is not applicable?. I have no received any bad comment and a "Reviewed-by:" tag from Haiyang Zhang. So, what is the reason for the "Not Applicable" state?. Regards, Len
Re: [PATCH] net: mana: Prefer struct_size over open coded arithmetic
Hi Kees, On Sat, Sep 18, 2021 at 06:51:51AM -0700, Kees Cook wrote: > > > On September 18, 2021 6:20:10 AM PDT, Len Baker wrote: > >Hi, > > > >On Sat, Sep 11, 2021 at 12:28:18PM +0200, Len Baker wrote: > >> As noted in the "Deprecated Interfaces, Language Features, Attributes, > >> and Conventions" documentation [1], size calculations (especially > >> multiplication) should not be performed in memory allocator (or similar) > >> function arguments due to the risk of them overflowing. This could lead > >> to values wrapping around and a smaller allocation being made than the > >> caller was expecting. Using those allocations could lead to linear > >> overflows of heap memory and other misbehaviors. > >> > >> So, use the struct_size() helper to do the arithmetic instead of the > >> argument "size + count * size" in the kzalloc() function. > >> > >> [1] > >> https://www.kernel.org/doc/html/v5.14/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > >> > >> Signed-off-by: Len Baker > >> --- > >> drivers/net/ethernet/microsoft/mana/hw_channel.c | 4 +--- > >> 1 file changed, 1 insertion(+), 3 deletions(-) > >> > >> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > >> b/drivers/net/ethernet/microsoft/mana/hw_channel.c > >> index 1a923fd0..0efdc6c3c32a 100644 > >> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > >> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > >> @@ -398,9 +398,7 @@ static int mana_hwc_alloc_dma_buf(struct > >> hw_channel_context *hwc, u16 q_depth, > >>int err; > >>u16 i; > >> > >> - dma_buf = kzalloc(sizeof(*dma_buf) + > >> -q_depth * sizeof(struct hwc_work_request), > >> -GFP_KERNEL); > >> + dma_buf = kzalloc(struct_size(dma_buf, reqs, q_depth), GFP_KERNEL); > >>if (!dma_buf) > >>return -ENOMEM; > >> > >> -- > >> 2.25.1 > >> > > > >I have received a email from the linux-media subsystem telling that this > >patch is not applicable. The email is the following: > > > >Hello, > > > >The following patch (submitted by you) has been updated in Patchwork: > > > > * linux-media: net: mana: Prefer struct_size over open coded arithmetic > > - > > http://patchwork.linuxtv.org/project/linux-media/patch/20210911102818.3804-1-len.ba...@gmx.com/ > > - for: Linux Media kernel patches > >was: New > >now: Not Applicable > > > >This email is a notification only - you do not need to respond. > > > >The question is: Why it is not applicable?. I have no received any bad > >comment > >and a "Reviewed-by:" tag from Haiyang Zhang. So, what is the reason for the > >"Not Applicable" state?. > > That is the "Media" subsystem patch tracker. The patch appears to be for > networking, so the Media tracker has marked it as "not applicable [to the > media subsystem]". > > The CC list for this patch seems rather wide (media, dri). I would have > expected only netdev. Were you using scripts/get_maintainer.pl for getting > addresses? Yes, my workflow is scripts/checkpatch.pl and then scripts/get_maintainer.pl before sending any patch :) Regards, Len > > -Kees
Re: [PATCH] net: mana: Prefer struct_size over open coded arithmetic
Hi Dexuan, On Sat, Sep 18, 2021 at 05:06:16PM +, Dexuan Cui wrote: > > From: Len Baker > > Sent: Saturday, September 18, 2021 6:20 AM > > ... > > I have received a email from the linux-media subsystem telling that this > > patch is not applicable. The email is the following: > > > > Regards, > > Len > > The patch is already in the net-next tree: > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=f11ee2ad25b22c2ee587045dd6999434375532f7 Thanks for the info. Regards, Len
[PATCH] drm/i915: Prefer struct_size over open coded arithmetic
As noted in the "Deprecated Interfaces, Language Features, Attributes,
and Conventions" documentation [1], size calculations (especially
multiplication) should not be performed in memory allocator (or similar)
function arguments due to the risk of them overflowing. This could lead
to values wrapping around and a smaller allocation being made than the
caller was expecting. Using those allocations could lead to linear
overflows of heap memory and other misbehaviors.
In this case these are not actually dynamic sizes: all the operands
involved in the calculation are constant values. However it is better to
refactor them anyway, just to keep the open-coded math idiom out of
code.
So, add at the end of the struct i915_syncmap a union with two flexible
array members (these arrays share the same memory layout). This is
possible using the new DECLARE_FLEX_ARRAY macro. And then, use the
struct_size() helper to do the arithmetic instead of the argument
"size + count * size" in the kmalloc and kzalloc() functions.
Also, take the opportunity to refactor the __sync_seqno and __sync_child
making them more readable.
This code was detected with the help of Coccinelle and audited and fixed
manually.
[1]
https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments
Signed-off-by: Len Baker
---
drivers/gpu/drm/i915/i915_syncmap.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_syncmap.c
b/drivers/gpu/drm/i915/i915_syncmap.c
index 60404dbb2e9f..a8d35491d05a 100644
--- a/drivers/gpu/drm/i915/i915_syncmap.c
+++ b/drivers/gpu/drm/i915/i915_syncmap.c
@@ -82,6 +82,10 @@ struct i915_syncmap {
* struct i915_syncmap *child[KSYNCMAP];
* };
*/
+ union {
+ DECLARE_FLEX_ARRAY(u32, seqno);
+ DECLARE_FLEX_ARRAY(struct i915_syncmap *, child);
+ };
};
/**
@@ -99,13 +103,13 @@ void i915_syncmap_init(struct i915_syncmap **root)
static inline u32 *__sync_seqno(struct i915_syncmap *p)
{
GEM_BUG_ON(p->height);
- return (u32 *)(p + 1);
+ return p->seqno;
}
static inline struct i915_syncmap **__sync_child(struct i915_syncmap *p)
{
GEM_BUG_ON(!p->height);
- return (struct i915_syncmap **)(p + 1);
+ return p->child;
}
static inline unsigned int
@@ -200,7 +204,7 @@ __sync_alloc_leaf(struct i915_syncmap *parent, u64 id)
{
struct i915_syncmap *p;
- p = kmalloc(sizeof(*p) + KSYNCMAP * sizeof(u32), GFP_KERNEL);
+ p = kmalloc(struct_size(p, seqno, KSYNCMAP), GFP_KERNEL);
if (unlikely(!p))
return NULL;
@@ -282,7 +286,7 @@ static noinline int __sync_set(struct i915_syncmap **root,
u64 id, u32 seqno)
unsigned int above;
/* Insert a join above the current layer */
- next = kzalloc(sizeof(*next) + KSYNCMAP * sizeof(next),
+ next = kzalloc(struct_size(next, child, KSYNCMAP),
GFP_KERNEL);
if (unlikely(!next))
return -ENOMEM;
--
2.25.1
Re: [PATCH] drm/i915: Prefer struct_size over open coded arithmetic
Hi, On Wed, Oct 27, 2021 at 06:06:28PM +0300, Jani Nikula wrote: > On Sat, 23 Oct 2021, Len Baker wrote: > > Sorry, but I'm missing something here. In linux-next this is the commit > > history of include/linux/stddef.h file: > > > > 3080ea5553cc stddef: Introduce DECLARE_FLEX_ARRAY() helper > > 50d7bd38c3aa stddef: Introduce struct_group() helper macro > > e7f18c22e6be stddef: Fix kerndoc for sizeof_field() and offsetofend() > > 4229a470175b stddef.h: Introduce sizeof_field() > > ... > > > > But in drm-tip this is the commit history: > > > > 4229a470175b stddef.h: Introduce sizeof_field() > > ... > > > > For this patch the DECLARE_FLEX_ARRAY() helper is needed. But the build > > fails due to the last tree commits for stddef.h file are not present. > > So, if I understand correctly, drm-tip is not up to date with linux-next. > > linux-next is an ephemeral integration branch for most arch, subsystem > and driver -next branches. > > drm-tip is an ephemeral integration branch for drm subsystem and driver > -next branches. > > They contain different sets of branches. They are constantly > rebuilt. They are not the end result or end goal. > > If a problem (or a solution, for that matter) only exists in the merge > of some of those branches, you can't actually fix it until such a merge > exists somewhere more permanent than an ephemeral integration branch. Ok, understood. Thanks for the clarification. Regards, Len
