On 11/6/21 15:05, Ajay Garg wrote:
Hi Pavel,
Thanks for the review.
> len = strlcpy(kbs, func_table[kb_func] ? : "", len);
^^^^^^^^^
len is reinitialized here, i.e len passed to kmalloc and len passed to
copy_to_user() can be different.
Sorry, I missed this part.
strlcpy() returns strlen() of source string (2nd argument), that's why
we need +1 here to pass null byte to user.
Am I missing something?
Seems things are more screwed.
I tried to see the behaviour, via a small program as below :
##########################
#include <stdio.h>
#include <bsd/string.h>
char a[10] = {0};
char b[] = "1234567890123456";
int main()
{
int len = strlcpy(a, b, sizeof(a));
printf("len = [%d]\n", len);
printf("a = [%s]\n", a);
return 0;
}
##########################
The result is :
##########################
len = [16]
a = [123456789]
##########################
As seen, len is *not equal* to the number of bytes actually copied.
(The bytes actually copied are 9 in number, plus 1 for the terminator,
as expected by strlcpy).
On re-reading the doc for strlcpy, it seems that strlcpy returns the
length of src it "intended* to copy, and not the bytes *actually
copied*. If so, then returned value of len is meaningless.
return value from strlcpy() is simply strlen(src)
lib/string.c:141
```
size_t strlcpy(char *dest, const char *src, size_t size)
{
size_t ret = strlen(src);
if (size) {
size_t len = (ret >= size) ? size - 1 : ret;
memcpy(dest, src, len);
dest[len] = '\0';
}
return ret;
}
```
I guess, it's what you mean by "intended to copy"
So, it seems following two changes should be made in the original code :
1.
len = strlcpy(kbs, func_table[kb_func] ? : "", len);
=>
strlcpy(kbs, func_table[kb_func] ? : "", len);
2.
ret = copy_to_user(user_kdgkb->kb_string, kbs, len) ?
-EFAULT : 0;
=>
ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
-EFAULT : 0;
In 1, we change to simply not using the returned value of strlcpy.
In 2, we change to using strlen(kbs) + 1, as the number of bytes to copy.
If I understood correctly, you are trying to prevent some kind of
overflow here, right?
I see, that strlen(func_table[i]) cannot be greater than
sizeof(user_kdgkb->kb_string) - 1.
vt_kdskbsent() is used to set func_table ptrs. It's called only from
vt_do_kdgkb_ioctl(). Buffer is allocated via
strndup_user(user_kdgkb->kb_string, sizeof(user_kdgkb->kb_string));
It means that maximum strlen() of returned pointer will be
sizeof(user_kdgkb->kb_string)) - 1, because 2nd argument is size *with*
null byte.
Back to KDGKBSENT handler: kbs is sizeof(user_kdgkb->kb_string)
allocated buffer and strlcpy() will return strlen(func_table[kb_func]),
which is guaranteed to be less than sizeof(user_kdgkb->kb_string). It
looks save to use strlcpy() return value here, because 3rd argument is
greater than strlen() of second argument.
Let me know if I am completely wrong here :)
With regards,
Pavel Skripkin
