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.per...@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,
--
js

Reply via email to