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:

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().

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

quoted

entirely from the kernel.

Signed-off-by: Len Baker <redacted>

...

quoted

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/ (local)

Regards,
Len

quoted

+				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