[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABgxDoJK01n+MbfEJOOz6+QoHzLEamooR_mz91nfqH5i0ok8fw@mail.gmail.com>
Date: Fri, 26 Feb 2021 16:09:48 +0100
From: Romain Perier <romain.perier@...il.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Kees Cook <keescook@...omium.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 17/20] vt: Manual replacement of the deprecated strlcpy()
with return values
Le ven. 26 févr. 2021 à 10:49, Jiri Slaby <jirislaby@...nel.org> a écrit :
> On 22. 02. 21, 16:12, Romain Perier wrote:
> > The strlcpy() reads the entire source buffer first, it is dangerous if
> > the source buffer lenght is unbounded or possibility non NULL-terminated.
>
> "length" and it's NUL, not NULL in this case.
>
> > It can lead to linear read overflows, crashes, etc...
> >
> > As recommended in the deprecated interfaces [1], it should be replaced
> > by strscpy.
> >
> > This commit replaces all calls to strlcpy that handle the return values
>
> s/that/which/ ?
> "handles"
> "value"
>
> > by the corresponding strscpy calls with new handling of the return
> > values (as it is quite different between the two functions).
>
> Sorry, I have hard times understand the whole sentence. Could you
> rephrase a bit?
>
> > [1]
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> >
> > Signed-off-by: Romain Perier <romain.perier@...il.com>
> > ---
> > drivers/tty/vt/keyboard.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > index 77638629c562..5e20c6c307e0 100644
> > --- a/drivers/tty/vt/keyboard.c
> > +++ b/drivers/tty/vt/keyboard.c
> > @@ -2067,9 +2067,12 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry
> __user *user_kdgkb, int perm)
> > return -ENOMEM;
> >
> > spin_lock_irqsave(&func_buf_lock, flags);
> > - len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> > + len = strscpy(kbs, func_table[kb_func] ? : "", len);
>
> func_table[kb_func] is NUL-terminated and kbs is of length len anyway,
> so this is only cosmetical.
>
> > spin_unlock_irqrestore(&func_buf_lock, flags);
> >
> > + if (len == -E2BIG)
> > + return -E2BIG;
> > +
>
> This can never happen, right?
>
> > ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
> > -EFAULT : 0;
> >
> >
>
>
Hello,
Yeah I will reword the commit message, I have realized that it might be
confusing in some cases. No it is
not only cosmetic, see my new commit message below (does it help ?):
"
Nowadays, strings copies are common in the kernel and strlcpy() is
widely used for this purpose. While being a very helpful function, this
has several problems:
- strlcpy() reads the entire source buffer first (since the return value
is meant to match that of strlen()). This read may exceed the destination
size limit. This can lead to linear read overflows if a source string is
not NUL-terminated.
- This is inefficient as it does not check for unaligned accesses,
copies the source into the destination with a simple byte copy and reads
the source buffer twice (even if the cache helps).
- Even when the use of strlcpy() is correct and its source buffer is
NUL-terminated, it might be an attack vector: a possible future security
breach could give the opportunity to modify the source buffer.
strscpy() instead checks for alignment constraints and, when the
conditions are met, copies word by word the sources into the destination
(while checking that it does not exceed both buffers). Its use should be
reasonably performant on all platforms since it uses the
asm/world-at-a-time.h API ranther than simple byte copy.
This commit replaces all calls to strlcpy() which handles error codes by
the corresponding call to strscpy(), while adjusting the error handling
(as it is quite different between the two functions).
[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
"
Regards,
Romain
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
