Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 8 Aug 2021 13:26:17 +0200
From:   Len Baker <len.baker@gmx.com>
To:     Joe Perches <joe@perches.com>, Robert Richter <rric@kernel.org>
Cc:     Borislav Petkov <bp@alien8.de>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Tony Luck <tony.luck@intel.com>,
        James Morse <james.morse@arm.com>,
        Len Baker <len.baker@gmx.com>,
        Kees Cook <keescook@chromium.org>,
        linux-hardening@vger.kernel.org, linux-edac@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] drivers/edac/edac_mc: Remove all strcpy() uses
Message-ID: <20210808112617.GA1927@titan>
References: <20210807155957.10069-1-len.baker@gmx.com>
 <ff02ffffdc130a772c01ec0edbf8d1e684b0730a.camel@perches.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <ff02ffffdc130a772c01ec0edbf8d1e684b0730a.camel@perches.com>
Precedence: bulk

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,
> > =A0			p =3D e->label;
> > =A0			*p =3D '\0';
> > =A0		} else {
> > -			if (p !=3D e->label) {
> > -				strcpy(p, OTHER_LABEL);
> > -				p +=3D strlen(OTHER_LABEL);
> > -			}
> > -			strcpy(p, dimm->label);
> > +			const char *text =3D (p !=3D e->label) ? OTHER_LABEL :
> > +				dimm->label;
> > +
> > +			strscpy(p, text, len);
> > +			len -=3D strlen(p);
> > =A0			p +=3D strlen(p);
>
> Perhaps this should use scnprintf rather than strscpy
> Something like:
> 			n +=3D scnprintf(buf + n, len - n, "%s",
> 				       p =3D=3D 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.bake=
r@gmx.com/

Regards,
Len