| Message ID | 20210222151231.22572-18-romain.perier@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Manual replacement of all strlcpy in favor of strscpy | expand |
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@gmail.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; > > thanks,
Le ven. 26 févr. 2021 à 10:49, Jiri Slaby <jirislaby@kernel.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@gmail.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
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); spin_unlock_irqrestore(&func_buf_lock, flags); + if (len == -E2BIG) + return -E2BIG; + ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ? -EFAULT : 0;
The strlcpy() reads the entire source buffer first, it is dangerous if the source buffer lenght is unbounded or possibility non NULL-terminated. 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 by the corresponding strscpy calls with new handling of the return values (as it is quite different between the two functions). [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy Signed-off-by: Romain Perier <romain.perier@gmail.com> --- drivers/tty/vt/keyboard.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)