linux-edac.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
@ 2021-08-07 15:59 Len Baker
  2021-08-07 17:09 ` Joe Perches
  2021-08-09  9:51 ` Robert Richter
  0 siblings, 2 replies; 11+ messages in thread
From: Len Baker @ 2021-08-07 15:59 UTC (permalink / raw)
  To: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Robert Richter
  Cc: Len Baker, Kees Cook, linux-hardening, linux-edac, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-07 15:59 [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses Len Baker
@ 2021-08-07 17:09 ` Joe Perches
  2021-08-08 11:26   ` Len Baker
  2021-08-09  9:51 ` Robert Richter
  1 sibling, 1 reply; 11+ messages in thread
From: Joe Perches @ 2021-08-07 17:09 UTC (permalink / raw)
  To: Len Baker, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Robert Richter
  Cc: Kees Cook, linux-hardening, linux-edac, linux-kernel

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




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-07 17:09 ` Joe Perches
@ 2021-08-08 11:26   ` Len Baker
  2021-08-09 10:05     ` Robert Richter
  0 siblings, 1 reply; 11+ messages in thread
From: Len Baker @ 2021-08-08 11:26 UTC (permalink / raw)
  To: Joe Perches, Robert Richter
  Cc: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Len Baker, Kees Cook, linux-hardening, linux-edac, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-07 15:59 [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses Len Baker
  2021-08-07 17:09 ` Joe Perches
@ 2021-08-09  9:51 ` Robert Richter
  2021-08-09 16:25   ` Len Baker
  1 sibling, 1 reply; 11+ messages in thread
From: Robert Richter @ 2021-08-09  9:51 UTC (permalink / raw)
  To: Len Baker
  Cc: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Kees Cook, linux-hardening, linux-edac, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-08 11:26   ` Len Baker
@ 2021-08-09 10:05     ` Robert Richter
  2021-08-09 17:18       ` Joe Perches
  0 siblings, 1 reply; 11+ messages in thread
From: Robert Richter @ 2021-08-09 10:05 UTC (permalink / raw)
  To: Len Baker
  Cc: Joe Perches, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Kees Cook, linux-hardening, linux-edac,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-09  9:51 ` Robert Richter
@ 2021-08-09 16:25   ` Len Baker
  0 siblings, 0 replies; 11+ messages in thread
From: Len Baker @ 2021-08-09 16:25 UTC (permalink / raw)
  To: Robert Richter
  Cc: Len Baker, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Kees Cook, linux-hardening, linux-edac,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-09 10:05     ` Robert Richter
@ 2021-08-09 17:18       ` Joe Perches
  2021-08-10  8:56         ` David Laight
  2021-08-10 14:36         ` Robert Richter
  0 siblings, 2 replies; 11+ messages in thread
From: Joe Perches @ 2021-08-09 17:18 UTC (permalink / raw)
  To: Robert Richter, Len Baker
  Cc: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Kees Cook, linux-hardening, linux-edac, linux-kernel

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.




^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-09 17:18       ` Joe Perches
@ 2021-08-10  8:56         ` David Laight
  2021-08-10 14:36         ` Robert Richter
  1 sibling, 0 replies; 11+ messages in thread
From: David Laight @ 2021-08-10  8:56 UTC (permalink / raw)
  To: 'Joe Perches', Robert Richter, Len Baker
  Cc: Borislav Petkov, Mauro Carvalho Chehab, Tony Luck, James Morse,
	Kees Cook, linux-hardening, linux-edac, linux-kernel

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)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-09 17:18       ` Joe Perches
  2021-08-10  8:56         ` David Laight
@ 2021-08-10 14:36         ` Robert Richter
  2021-08-10 15:02           ` Joe Perches
  1 sibling, 1 reply; 11+ messages in thread
From: Robert Richter @ 2021-08-10 14:36 UTC (permalink / raw)
  To: Joe Perches
  Cc: Len Baker, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Kees Cook, linux-hardening, linux-edac,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-10 14:36         ` Robert Richter
@ 2021-08-10 15:02           ` Joe Perches
  2021-08-11  7:40             ` Robert Richter
  0 siblings, 1 reply; 11+ messages in thread
From: Joe Perches @ 2021-08-10 15:02 UTC (permalink / raw)
  To: Robert Richter
  Cc: Len Baker, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Kees Cook, linux-hardening, linux-edac,
	linux-kernel

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.




^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
  2021-08-10 15:02           ` Joe Perches
@ 2021-08-11  7:40             ` Robert Richter
  0 siblings, 0 replies; 11+ messages in thread
From: Robert Richter @ 2021-08-11  7:40 UTC (permalink / raw)
  To: Joe Perches
  Cc: Len Baker, Borislav Petkov, Mauro Carvalho Chehab, Tony Luck,
	James Morse, Kees Cook, linux-hardening, linux-edac,
	linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-11  7:40 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-07 15:59 [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses Len Baker
2021-08-07 17:09 ` Joe Perches
2021-08-08 11:26   ` Len Baker
2021-08-09 10:05     ` Robert Richter
2021-08-09 17:18       ` Joe Perches
2021-08-10  8:56         ` David Laight
2021-08-10 14:36         ` Robert Richter
2021-08-10 15:02           ` Joe Perches
2021-08-11  7:40             ` Robert Richter
2021-08-09  9:51 ` Robert Richter
2021-08-09 16:25   ` Len Baker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).