[PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Len Baker]

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

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);

--
2.25.1

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Joe Perches]

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);

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Len Baker]

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

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Robert Richter]

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);
>  		}
> 

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Robert Richter]

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

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Len Baker]

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);
> >  		}
> >

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Joe Perches]

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.

RE: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[David Laight]

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)

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Robert Richter]

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

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Joe Perches]

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.

Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses

[Robert Richter]

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