Messages in this thread | | | From | Ajay Garg <> | Date | Sat, 6 Nov 2021 17:35:49 +0530 | Subject | Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user |
| |
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.
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.
Kindly know your thoughts.
Thanks and Regards, Ajay
| |