[PATCH] kobject: Replace strlcpy with strscpy

[PATCH] kobject: Replace strlcpy with strscpy

[Azeem Shaikh]

strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit.
This is both inefficient and can lead to linear read
overflows if a source string is not NUL-terminated [1].
In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().

Direct replacement is safe here since return value of -errno
is used to check for truncation instead of sizeof(dest).

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
 lib/kobject_uevent.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7c44b7ae4c5c..e5497fa0a2d2 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -254,8 +254,8 @@ static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem)
 	int buffer_size = sizeof(env->buf) - env->buflen;
 	int len;
 
-	len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size);
-	if (len >= buffer_size) {
+	len = strscpy(&env->buf[env->buflen], subsystem, buffer_size);
+	if (len < 0) {
 		pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n",
 			buffer_size, len);
 		return -ENOMEM;
-- 
2.41.0.255.g8b1d071c50-goog

RE: [PATCH] kobject: Replace strlcpy with strscpy

[David Laight]

From: Azeem Shaikh
> Sent: 03 July 2023 19:05
> 
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit.
> This is both inefficient and can lead to linear read
> overflows if a source string is not NUL-terminated [1].
> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> 
> Direct replacement is safe here since return value of -errno
> is used to check for truncation instead of sizeof(dest).
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
>  lib/kobject_uevent.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 7c44b7ae4c5c..e5497fa0a2d2 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -254,8 +254,8 @@ static int init_uevent_argv(struct kobj_uevent_env *env, const char *subsystem)
>  	int buffer_size = sizeof(env->buf) - env->buflen;
>  	int len;
> 
> -	len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size);
> -	if (len >= buffer_size) {
> +	len = strscpy(&env->buf[env->buflen], subsystem, buffer_size);
> +	if (len < 0) {
>  		pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n",
>  			buffer_size, len);
>  		return -ENOMEM;

The size in the error message is now wrong.
It has to be said that mostly all the strings that get copied
in the kernel are '\0' terminated - so maybe it is all moot.
OTOH printing (at least some of) the string that didn't fit
is a lot more useful than its length.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] kobject: Replace strlcpy with strscpy

[Azeem Shaikh]

On Mon, Jul 10, 2023 at 9:13 AM David Laight <David.Laight@aculab.com> wrote:
>
> >       int len;
> >
> > -     len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size);
> > -     if (len >= buffer_size) {
> > +     len = strscpy(&env->buf[env->buflen], subsystem, buffer_size);
> > +     if (len < 0) {
> >               pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n",
> >                       buffer_size, len);
> >               return -ENOMEM;
>
> The size in the error message is now wrong.

Thanks for catching this.

> It has to be said that mostly all the strings that get copied
> in the kernel are '\0' terminated - so maybe it is all moot.
> OTOH printing (at least some of) the string that didn't fit
> is a lot more useful than its length.

How about printing out strlen(subsystem) along with the entire value
of @subsystem? So that the warn reads:

pr_warn("init_uevent_argv: buffer size of %d too small for %s, needed
%d\n", buffer_size, subsystem, strlen(subsystem));

Does that seem better?

RE: [PATCH] kobject: Replace strlcpy with strscpy

[David Laight]

From: Azeem Shaikh
> Sent: 10 July 2023 19:07
> 
> On Mon, Jul 10, 2023 at 9:13 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > >       int len;
> > >
> > > -     len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size);
> > > -     if (len >= buffer_size) {
> > > +     len = strscpy(&env->buf[env->buflen], subsystem, buffer_size);
> > > +     if (len < 0) {
> > >               pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n",
> > >                       buffer_size, len);
> > >               return -ENOMEM;
> >
> > The size in the error message is now wrong.
> 
> Thanks for catching this.
> 
> > It has to be said that mostly all the strings that get copied
> > in the kernel are '\0' terminated - so maybe it is all moot.
> > OTOH printing (at least some of) the string that didn't fit
> > is a lot more useful than its length.
> 
> How about printing out strlen(subsystem) along with the entire value
> of @subsystem? So that the warn reads:
> 
> pr_warn("init_uevent_argv: buffer size of %d too small for %s, needed
> %d\n", buffer_size, subsystem, strlen(subsystem));
> 
> Does that seem better?

Not with the justification for not using strlcpy() :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] kobject: Replace strlcpy with strscpy

[Kees Cook]

On Tue, Jul 11, 2023 at 08:14:30AM +0000, David Laight wrote:
> From: Azeem Shaikh
> > Sent: 10 July 2023 19:07
> > 
> > On Mon, Jul 10, 2023 at 9:13 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > >       int len;
> > > >
> > > > -     len = strlcpy(&env->buf[env->buflen], subsystem, buffer_size);
> > > > -     if (len >= buffer_size) {
> > > > +     len = strscpy(&env->buf[env->buflen], subsystem, buffer_size);
> > > > +     if (len < 0) {
> > > >               pr_warn("init_uevent_argv: buffer size of %d too small, needed %d\n",
> > > >                       buffer_size, len);
> > > >               return -ENOMEM;
> > >
> > > The size in the error message is now wrong.
> > 
> > Thanks for catching this.
> > 
> > > It has to be said that mostly all the strings that get copied
> > > in the kernel are '\0' terminated - so maybe it is all moot.
> > > OTOH printing (at least some of) the string that didn't fit
> > > is a lot more useful than its length.
> > 
> > How about printing out strlen(subsystem) along with the entire value
> > of @subsystem? So that the warn reads:
> > 
> > pr_warn("init_uevent_argv: buffer size of %d too small for %s, needed
> > %d\n", buffer_size, subsystem, strlen(subsystem));
> > 
> > Does that seem better?

Yeah, that'll retain the intention of the warning. It shouldn't really
even be possible to hit that warning, so I don't think we need to worry
about the "extra" call to strlen().

> Not with the justification for not using strlcpy() :-)

What?

-Kees

-- 
Kees Cook

RE: [PATCH] kobject: Replace strlcpy with strscpy

[David Laight]

From: Kees Cook <keescook@chromium.org>
> Sent: 13 July 2023 00:53
....
> > > pr_warn("init_uevent_argv: buffer size of %d too small for %s, needed
> > > %d\n", buffer_size, subsystem, strlen(subsystem));
> > >
> > > Does that seem better?
> 
> Yeah, that'll retain the intention of the warning. It shouldn't really
> even be possible to hit that warning, so I don't think we need to worry
> about the "extra" call to strlen().
> 
> > Not with the justification for not using strlcpy() :-)
> 
> What?

The commit message says that strlcpy() isn't used because
of possible 'read beyond buffer end' (etc) if it isn't
'\0' terminated.

But here if (extremely unlikely) the source isn't terminated
then the extra reads get done anyway.
So the commit message just doesn't match the change.

I'd guess that it is possible for there to be insufficient space.
Probably because other strings have filled the buffer.
The error message might be better as:
	"insufficient buffer space (%u left) for %s\n",
		buffer_size, subsystem);

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)