Message ID | 20230703180528.3709258-1-azeemshaikh38@gmail.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | kobject: Replace strlcpy with strscpy | expand |
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)
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?
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)
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
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)
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;
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(-)