| Message ID | 20210807155957.10069-1-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drivers/edac/edac_mc: Remove all strcpy() uses | expand |
On Sat, 2021-08-07 at 17:59 +0200, 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(). Probably better to change the commit subject to something like what is generally used by the subsystem. Maybe: EDAC/mc: Convert strcpy to strscpy or EDAC/mc: Prefer strscpy over strcpy and also: > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c [] > @@ -1113,11 +1115,11 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > p = e->label; > *p = '\0'; > } else { > - if (p != e->label) { > - strcpy(p, OTHER_LABEL); > - p += strlen(OTHER_LABEL); > - } > - strcpy(p, dimm->label); > + const char *text = (p != e->label) ? OTHER_LABEL : > + dimm->label; > + > + strscpy(p, text, len); > + len -= strlen(p); > p += strlen(p); Perhaps this should use scnprintf rather than strscpy Something like: n += scnprintf(buf + n, len - n, "%s", p == e->label ? dim->label : OTHER_LABEL);
Hi, On Sat, Aug 07, 2021 at 10:09:35AM -0700, Joe Perches wrote: > On Sat, 2021-08-07 at 17:59 +0200, 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(). > > Probably better to change the commit subject to something like > what is generally used by the subsystem. > > Maybe: > EDAC/mc: Convert strcpy to strscpy > or > EDAC/mc: Prefer strscpy over strcpy Ok, no problem. I like the second one. > and also: > > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > [] > > @@ -1113,11 +1115,11 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > > p = e->label; > > *p = '\0'; > > } else { > > - if (p != e->label) { > > - strcpy(p, OTHER_LABEL); > > - p += strlen(OTHER_LABEL); > > - } > > - strcpy(p, dimm->label); > > + const char *text = (p != e->label) ? OTHER_LABEL : > > + dimm->label; > > + > > + strscpy(p, text, len); > > + len -= strlen(p); > > p += strlen(p); > > Perhaps this should use scnprintf rather than strscpy > Something like: > n += scnprintf(buf + n, len - n, "%s", > p == e->label ? dim->label : OTHER_LABEL); > In the first version [1] the scnprintf was used but Robert Richter don't see any benefit compared with the current implementation. [1] https://lore.kernel.org/linux-hardening/20210725162954.9861-1-len.baker@gmx.com/ Regards, Len
On 07.08.21 17:59:57, Len Baker wrote: > @@ -1113,11 +1115,11 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > p = e->label; > *p = '\0'; > } else { > - if (p != e->label) { > - strcpy(p, OTHER_LABEL); > - p += strlen(OTHER_LABEL); > - } > - strcpy(p, dimm->label); > + const char *text = (p != e->label) ? OTHER_LABEL : > + dimm->label; > + > + strscpy(p, text, len); > + len -= strlen(p); The logic looks broken and dimm labels are not properly copied (the code should add an " or " separator between labels). -Robert > p += strlen(p); > } >
On 08.08.21 13:26:17, Len Baker wrote: > > Perhaps this should use scnprintf rather than strscpy > > Something like: > > n += scnprintf(buf + n, len - n, "%s", > > p == e->label ? dim->label : OTHER_LABEL); > > > In the first version [1] the scnprintf was used but Robert Richter don't > see any benefit compared with the current implementation. > > [1] https://lore.kernel.org/linux-hardening/20210725162954.9861-1-len.baker@gmx.com/ Reason is that there is the assumption that p must always point at the end of the string and its trailing zero byte. I am not opposed using the string function's return code instead of strlen() to get the length. But why using formated output if strscpy() can be used? -Robert
Hi, On Mon, Aug 09, 2021 at 11:51:54AM +0200, Robert Richter wrote: > On 07.08.21 17:59:57, Len Baker wrote: > > > @@ -1113,11 +1115,11 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > > p = e->label; > > *p = '\0'; > > } else { > > - if (p != e->label) { > > - strcpy(p, OTHER_LABEL); > > - p += strlen(OTHER_LABEL); > > - } > > - strcpy(p, dimm->label); > > + const char *text = (p != e->label) ? OTHER_LABEL : > > + dimm->label; > > + > > + strscpy(p, text, len); > > + len -= strlen(p); > > The logic looks broken and dimm labels are not properly copied (the > code should add an " or " separator between labels). Apologies. My bad. Regards, Len > > > p += strlen(p); > > } > >
On Mon, 2021-08-09 at 12:05 +0200, Robert Richter wrote: > On 08.08.21 13:26:17, Len Baker wrote: > > > > Perhaps this should use scnprintf rather than strscpy > > > Something like: > > > n += scnprintf(buf + n, len - n, "%s", > > > p == e->label ? dim->label : OTHER_LABEL); > > > > > In the first version [1] the scnprintf was used but Robert Richter don't > > see any benefit compared with the current implementation. > > > > [1] https://lore.kernel.org/linux-hardening/20210725162954.9861-1-len.baker@gmx.com/ > > Reason is that there is the assumption that p must always point at the > end of the string and its trailing zero byte. I am not opposed using > the string function's return code instead of strlen() to get the > length. But why using formated output if strscpy() can be used? strscpy and scnprintf have different return values and it's simpler and much more common to use scnprintf for appended strings that are limited to a specific buffer length.
From: Joe Perches > Sent: 09 August 2021 18:19 > > On Mon, 2021-08-09 at 12:05 +0200, Robert Richter wrote: > > On 08.08.21 13:26:17, Len Baker wrote: > > > > > > Perhaps this should use scnprintf rather than strscpy > > > > Something like: > > > > n += scnprintf(buf + n, len - n, "%s", > > > > p == e->label ? dim->label : OTHER_LABEL); > > > > > > > In the first version [1] the scnprintf was used but Robert Richter don't > > > see any benefit compared with the current implementation. > > > > > > [1] https://lore.kernel.org/linux-hardening/20210725162954.9861-1-len.baker@gmx.com/ > > > > Reason is that there is the assumption that p must always point at the > > end of the string and its trailing zero byte. I am not opposed using > > the string function's return code instead of strlen() to get the > > length. But why using formated output if strscpy() can be used? > > strscpy and scnprintf have different return values and it's simpler > and much more common to use scnprintf for appended strings that are > limited to a specific buffer length. scnprintf() will be a lot slower, but has a much better return value than most of the strxxxcpy() functions. The only slight problem is that you can't differentiate overflow from a max-length output. Trouble is fixing that adds 'yet another set of functions'. Clearly we need the yellow with purple stripe ones :-) Probably: offset = xxx(buf, len, offset, ......) where offset == len on truncation. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On 09.08.21 10:18:58, Joe Perches wrote: > strscpy and scnprintf have different return values and it's simpler > and much more common to use scnprintf for appended strings that are > limited to a specific buffer length. Calculating the bytes written from the return value is a oneliner. -Robert
On Tue, 2021-08-10 at 16:36 +0200, Robert Richter wrote: > On 09.08.21 10:18:58, Joe Perches wrote: > > > strscpy and scnprintf have different return values and it's simpler > > and much more common to use scnprintf for appended strings that are > > limited to a specific buffer length. > > Calculating the bytes written from the return value is a oneliner. Not really. You still have to test for strscpy's possible return of -E2BIG.
On 10.08.21 08:02:17, Joe Perches wrote: > On Tue, 2021-08-10 at 16:36 +0200, Robert Richter wrote: > > On 09.08.21 10:18:58, Joe Perches wrote: > > > > > strscpy and scnprintf have different return values and it's simpler > > > and much more common to use scnprintf for appended strings that are > > > limited to a specific buffer length. > > > > Calculating the bytes written from the return value is a oneliner. > > Not really. > You still have to test for strscpy's possible return of -E2BIG. I thought of: num = strscpy(p, OTHER_LABEL, len); num = num < 0 ? len : num; len -= num; p += num; Clearly, this does not look nice, esp. if this is repeated in the code. That's why I prefer the strlen(p) implementation: strscpy(p, OTHER_LABEL, len); len -= strlen(p); p += strlen(p); -Robert
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index f6d462d0be2d..0cdb1e9320ba 100644 --- a/drivers/edac/edac_mc.c +++ b/drivers/edac/edac_mc.c @@ -1032,6 +1032,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, int i, n_labels = 0; struct edac_raw_error_desc *e = &mci->error_desc; bool any_memory = true; + size_t len; edac_dbg(3, "MC%d\n", mci->mc_idx); @@ -1086,6 +1087,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, */ p = e->label; *p = '\0'; + len = sizeof(e->label); mci_for_each_dimm(mci, dimm) { if (top_layer >= 0 && top_layer != dimm->location[0]) @@ -1113,11 +1115,11 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, p = e->label; *p = '\0'; } else { - if (p != e->label) { - strcpy(p, OTHER_LABEL); - p += strlen(OTHER_LABEL); - } - strcpy(p, dimm->label); + const char *text = (p != e->label) ? OTHER_LABEL : + dimm->label; + + strscpy(p, text, len); + len -= strlen(p); p += strlen(p); } @@ -1140,9 +1142,9 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, } if (any_memory) - strcpy(e->label, "any memory"); + strscpy(e->label, "any memory", sizeof(e->label)); else if (!*e->label) - strcpy(e->label, "unknown memory"); + strscpy(e->label, "unknown memory", sizeof(e->label)); edac_inc_csrow(e, row, chan);
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(). This is a previous step in the path to remove the strcpy() function entirely from the kernel. Signed-off-by: Len Baker <len.baker@gmx.com> --- This is a task of the KSPP [1] [1] https://github.com/KSPP/linux/issues/88 Changelog v1 -> v2 - Use the strscpy() instead of scnprintf() to add labels and follow a code pattern more similar to the current one (advance "p" and decrement "left") (Robert Richter). Changelog v2 -> v3 - Rename the "left" variable to "len" (Robert Richter). - Use strlen(p) instead of strlen(OTHER_LABEL) to decrement "len" and increment "p" as otherwise "left" could underflow and p overflow (Robert Richter). Previous versions: v1 https://lore.kernel.org/linux-hardening/20210725162954.9861-1-len.baker@gmx.com/ v2 https://lore.kernel.org/linux-hardening/20210801143558.12674-1-len.baker@gmx.com/ drivers/edac/edac_mc.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) -- 2.25.1