| Message ID | 20210814075527.5999-1-len.baker@gmx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] EDAC/mc: Prefer strscpy over strcpy | expand |
On Sat, Aug 14, 2021 at 09:55:27AM +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(). > > This is a previous step in the path to remove the strcpy() function "previous step"? > entirely from the kernel. > > Signed-off-by: Len Baker <len.baker@gmx.com> ... > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > index f6d462d0be2d..7aea6c502316 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]) > @@ -1114,10 +1116,12 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > *p = '\0'; > } else { > if (p != e->label) { > - strcpy(p, OTHER_LABEL); > - p += strlen(OTHER_LABEL); > + strscpy(p, OTHER_LABEL, len); Hm, maybe I'm missing something but looking at that strscpy() definition, why aren't you doing: num = strscpy(p, OTHER_LABEL, len); if (num < 0) /* just in case */ break; len -= num; p += num; since that function supposedly returns the number of chars copied. > + len -= strlen(p); > + p += strlen(p); > } > - strcpy(p, dimm->label); > + strscpy(p, dimm->label, len); > + len -= strlen(p); > p += strlen(p); Ditto. Thx.
Hi Borislav, On Mon, Aug 23, 2021 at 07:30:34PM +0200, Borislav Petkov wrote: > On Sat, Aug 14, 2021 at 09:55:27AM +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(). > > > > This is a previous step in the path to remove the strcpy() function > > "previous step"? This is a task of the KSPP [1] and the main reason is to clean up the proliferation of str*cpy functions in the kernel. [1] https://github.com/KSPP/linux/issues/88 > > entirely from the kernel. > > > > Signed-off-by: Len Baker <len.baker@gmx.com> > > ... > > > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > > index f6d462d0be2d..7aea6c502316 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]) > > @@ -1114,10 +1116,12 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, > > *p = '\0'; > > } else { > > if (p != e->label) { > > - strcpy(p, OTHER_LABEL); > > - p += strlen(OTHER_LABEL); > > + strscpy(p, OTHER_LABEL, len); > > Hm, maybe I'm missing something but looking at that strscpy() > definition, why aren't you doing: > > num = strscpy(p, OTHER_LABEL, len); > if (num < 0) > /* just in case */ > break; > > len -= num; > p += num; > > since that function supposedly returns the number of chars copied. Yes, you are right. The same discussion happened in the v3 review [2] and I agree with the reasons that Robert Richter exposed. Using the strlen() implementation it is not necessary to check the return code of strcpy and we can assume a silent truncation. [2] https://lore.kernel.org/linux-hardening/YRN+8u59lJ6MWsOL@rric.localdomain/ Regards, Len > > + len -= strlen(p); > > + p += strlen(p); > > } > > - strcpy(p, dimm->label); > > + strscpy(p, dimm->label, len); > > + len -= strlen(p); > > p += strlen(p); > > Ditto. > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Aug 24, 2021 at 12:28:07PM +0200, Len Baker wrote: > This is a task of the KSPP [1] and the main reason is to clean up the > proliferation of str*cpy functions in the kernel. That I understood - you prominently explain where the patches stem from. What I can't parse is that formulation "previous step". What previous step do you mean? > Yes, you are right. The same discussion happened in the v3 review [2] and > I agree with the reasons that Robert Richter exposed. Using the strlen() > implementation it is not necessary to check the return code of strcpy and > we can assume a silent truncation. > > [2] https://lore.kernel.org/linux-hardening/YRN+8u59lJ6MWsOL@rric.localdomain/ Ok, looking at the asm, gcc is actually smart enough not to call strlen() twice on the same buffer. But then there's this in the strscpy() kernel-doc comment: "The destination buffer is always NUL terminated, unless it's zero-sized." so looking at the code, we're merrily decrementing len but nothing's checking whether len can become 0. Because if it does, strscpy() will do: if (count == 0 || WARN_ON_ONCE(count > INT_MAX)) return -E2BIG; so if p ends up pointing to something which is *not* '\0', strlen() will go off into the weeds. So I don't care if it doesn't look just as nice - it better be correct in all cases first. Thx.
On Tue, 2021-08-24 at 20:26 +0200, Borislav Petkov wrote: > On Tue, Aug 24, 2021 at 12:28:07PM +0200, Len Baker wrote: > > This is a task of the KSPP [1] and the main reason is to clean up the > > proliferation of str*cpy functions in the kernel. > > That I understood - you prominently explain where the patches stem from. > > What I can't parse is that formulation "previous step". What previous > step do you mean? > > > Yes, you are right. The same discussion happened in the v3 review [2] and > > I agree with the reasons that Robert Richter exposed. Using the strlen() > > implementation it is not necessary to check the return code of strcpy and > > we can assume a silent truncation. > > > > [2] https://lore.kernel.org/linux-hardening/YRN+8u59lJ6MWsOL@rric.localdomain/ > > Ok, looking at the asm, gcc is actually smart enough not to call > strlen() twice on the same buffer. > > But then there's this in the strscpy() kernel-doc comment: > > "The destination buffer is always NUL terminated, unless it's > zero-sized." > > so looking at the code, we're merrily decrementing len but nothing's > checking whether len can become 0. Because if it does, strscpy() will > do: > > if (count == 0 || WARN_ON_ONCE(count > INT_MAX)) > return -E2BIG; > > so if p ends up pointing to something which is *not* '\0', strlen() will > go off into the weeds. > > So I don't care if it doesn't look just as nice - it better be correct > in all cases first. It's all somehat unnecessary as it seems it's guaranteed not to overflow. $ git grep -n -w OTHER_LABEL next-20210820 next-20210820:drivers/edac/edac_mc.c:1118: strcpy(p, OTHER_LABEL); next-20210820:drivers/edac/edac_mc.c:1119: p += strlen(OTHER_LABEL); next-20210820:include/linux/edac.h:57:#define OTHER_LABEL " or " next-20210820:include/linux/edac.h:470: char label[(EDAC_MC_LABEL_LEN + 1 + sizeof(OTHER_LABEL)) * EDAC_MAX_LABELS]; Also the array size of char label is too large. First by 1 * EDAC_MAX_LABELS as sizeof(OTHER_LABEL) is 5 not 4 and second there can only be EDAC_MAX_LABELS - 1 uses of OTHER_LABEL. And I would remove the define for OTHER_LABEL and use " or " directly. Lastly, this could be easier to understand using stpcpy and/or scnprintf.
From: Borislav Petkov > Sent: 24 August 2021 19:26 .. > so looking at the code, we're merrily decrementing len but nothing's > checking whether len can become 0. Because if it does, strscpy() will > do: > > if (count == 0 || WARN_ON_ONCE(count > INT_MAX)) > return -E2BIG; That -E2BIG is going to break something. It means that you always have to do an error check whenever you use the return value of strscpy(). Anything that does: offset += strscpy(...) is broken. It really wasn't a good idea for reporting 'truncated'. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi, On Tue, Aug 24, 2021 at 08:26:10PM +0200, Borislav Petkov wrote: > On Tue, Aug 24, 2021 at 12:28:07PM +0200, Len Baker wrote: > > This is a task of the KSPP [1] and the main reason is to clean up the > > proliferation of str*cpy functions in the kernel. > > That I understood - you prominently explain where the patches stem from. > > What I can't parse is that formulation "previous step". What previous > step do you mean? Well, the main purpose is to clean up the proliferation of str*cpy functions. One task is to remove the strcpy uses: The first step (previous step) would be to remove all the strcpy uses. Then, as a second step remove all the strcpy implementations. I hope that this clarify your question. Regards, Len
On Fri, Aug 27, 2021 at 07:36:33PM +0200, Len Baker wrote: > Well, the main purpose is to clean up the proliferation of str*cpy functions. > One task is to remove the strcpy uses: The first step (previous step) would > be to remove all the strcpy uses. Then, as a second step remove all the > strcpy implementations. > > I hope that this clarify your question. Yes, it does. Now lemme clarify why I'm asking: when your patch is committed to the kernel tree and someone reads its commit message months or even years from now - and those who do that are mostly maintainers trying to figure out why stuff was done the way it was - they will read: "This is a previous step in the path to remove the strcpy() function entirely from the kernel." and wonder what previous step that is what the following step is... So, long story short, your commit message should be complete on its own and understandable without any references to things which might not be as clear and self-evident in the future as they are now. Makes sense? Also, if you're wondering if you should send the patch with the error checking of strscpy() added, as I requested, even if it might look superfluous now, yes you should. Even if it looks impossible now, we might change some of those defines in the future and forget to touch the logic which generates e->label and we might end up exhausting that string. So it would be a lot more robust if something would catch that change, albeit seemingly redundant now. I sincerely hope that clears up things. Thx.
On Fri, 2021-08-27 at 19:54 +0200, Borislav Petkov wrote: > So it would be a lot more robust if something would catch that change, > albeit seemingly redundant now. I still think scnprintf is _way_ more common and intelligible as a construct than this odd strscpy with required error checking.
Hi, On Fri, Aug 27, 2021 at 07:54:07PM +0200, Borislav Petkov wrote: > On Fri, Aug 27, 2021 at 07:36:33PM +0200, Len Baker wrote: > > Well, the main purpose is to clean up the proliferation of str*cpy functions. > > One task is to remove the strcpy uses: The first step (previous step) would > > be to remove all the strcpy uses. Then, as a second step remove all the > > strcpy implementations. > > > > I hope that this clarify your question. > > Yes, it does. > > Now lemme clarify why I'm asking: when your patch is committed to the > kernel tree and someone reads its commit message months or even years > from now - and those who do that are mostly maintainers trying to figure > out why stuff was done the way it was - they will read: > > "This is a previous step in the path to remove the strcpy() function > entirely from the kernel." > > and wonder what previous step that is what the following step is... > > So, long story short, your commit message should be complete on its own > and understandable without any references to things which might not be > as clear and self-evident in the future as they are now. > > Makes sense? Ok, understood. Thanks for the advise and guidance. > > Also, if you're wondering if you should send the patch with the error > checking of strscpy() added, as I requested, even if it might look > superfluous now, yes you should. > > Even if it looks impossible now, we might change some of those defines > in the future and forget to touch the logic which generates e->label and > we might end up exhausting that string. > > So it would be a lot more robust if something would catch that change, > albeit seemingly redundant now. > > I sincerely hope that clears up things. Yes, it clears up things. However I think the same that Joe: From Joe Perches: [...] I still think scnprintf is _way_ more common and intelligible as a construct than this odd strscpy with required error checking. [...] So, I will send a new version for review with the commit message updated and using the scnprintf. This way we can discuss using a real patch. Anyway thanks for the review. Regards, Len
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c index f6d462d0be2d..7aea6c502316 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]) @@ -1114,10 +1116,12 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type, *p = '\0'; } else { if (p != e->label) { - strcpy(p, OTHER_LABEL); - p += strlen(OTHER_LABEL); + strscpy(p, OTHER_LABEL, len); + len -= strlen(p); + p += strlen(p); } - strcpy(p, dimm->label); + strscpy(p, dimm->label, len); + len -= strlen(p); p += strlen(p); } @@ -1140,9 +1144,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> --- Hi, Following the comments in the previous version I don't use the scnprintf function avoiding speed penalties. Instead I use the strscpy. Thanks, Len 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). Changelog v3 -> v4 - Change the commit subject (Joe Perches). - Fix broken logic (Robert Richter). - Rebase against v5.14-rc5. 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/ v3 https://lore.kernel.org/linux-hardening/20210807155957.10069-1-len.baker@gmx.com/ drivers/edac/edac_mc.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) -- 2.25.1