Messages in this thread | | | Date | Sat, 6 Nov 2021 15:39:12 +0300 | Subject | Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user | From | Pavel Skripkin <> |
| |
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
| |