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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
From: Kees Cook <[email protected]>
> 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)