Both (statically-allocated) "user_kdgkb->kb_string" and
(dynamically-allocated) "kbs" are of length "len", so we must
not copy more than "len" bytes.
Signed-off-by: Ajay Garg <[email protected]>
---
drivers/tty/vt/keyboard.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index c7fbbcdcc346..dfef7de8a057 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2070,7 +2070,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
len = strlcpy(kbs, func_table[kb_func] ? : "", len);
spin_unlock_irqrestore(&func_buf_lock, flags);
- ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
+ ret = copy_to_user(user_kdgkb->kb_string, kbs, len) ?
-EFAULT : 0;
break;
--
2.30.2
Hi, Ajay!
On 11/6/21 12:20, Ajay Garg wrote:
> Both (statically-allocated) "user_kdgkb->kb_string" and
> (dynamically-allocated) "kbs" are of length "len", so we must
> not copy more than "len" bytes.
>
> Signed-off-by: Ajay Garg <[email protected]>
> ---
> drivers/tty/vt/keyboard.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index c7fbbcdcc346..dfef7de8a057 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2070,7 +2070,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
> 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.
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?
> spin_unlock_irqrestore(&func_buf_lock, flags);
>
> - ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
> + ret = copy_to_user(user_kdgkb->kb_string, kbs, len) ?
> -EFAULT : 0;
>
> break;
>
With regards,
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
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
From: Pavel Skripkin
> Sent: 06 November 2021 11:24
>
> Hi, Ajay!
>
> On 11/6/21 12:20, Ajay Garg wrote:
> > Both (statically-allocated) "user_kdgkb->kb_string" and
> > (dynamically-allocated) "kbs" are of length "len", so we must
> > not copy more than "len" bytes.
> >
> > Signed-off-by: Ajay Garg <[email protected]>
> > ---
> > drivers/tty/vt/keyboard.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> > index c7fbbcdcc346..dfef7de8a057 100644
> > --- a/drivers/tty/vt/keyboard.c
> > +++ b/drivers/tty/vt/keyboard.c
> > @@ -2070,7 +2070,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
> > 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.
>
> 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?
You want strscpy() - returns the number of characters/bytes it copied.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Thanks Pavel, Andy, David for the help.
Andy,
There is no compilation/runtime blocker.
There were warnings reported by smatch.
My intention is to make the method "vt_do_kdgkb_ioctl" bullet-proof in
itself, without depending upon external clients.
Pavel has explained that currently things are fine, as per :
https://lore.kernel.org/linux-serial/[email protected]/T/#m740fffb7c6ee52fdc98b9ef0b4e32a060b6a3be3
but it seems that there is a big flaw - we are dependent on the length
of "func_table[kb_func]" being ok. If func_table[kb_func] goes awry,
the method will cause overflow.
Since func_table[kb_func]" is not managed by the method, so the method
must not depend on func_table[kb_func]" length-correctness. Instead,
"vt_do_kdgkb_ioctl" must ensure no overflow, without depending how
external entities (func_table[kb_func] behave.
The issue with strlcpy, along with a potential "fix", has been explained in :
https://lore.kernel.org/linux-serial/[email protected]/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1
David has provided a simpler fix (usage of strscpy), as in :
https://lore.kernel.org/linux-serial/[email protected]/T/#m63dab1137e593f2030920a53272f71866b442f40
So, we could go with one of the above changes (mine/David's), or
nothing at all (since there is no blocker).
I vote for David's strscpy "fix", as it is simple, and does away with
the dependency on the length of "func_table[kb_func]".
Would like to know what the maintainers think.
If there is a consensus that the method "vt_do_kdgkb_ioctl" be made
bullet-proof in itself, please let me know, I will float the next
version of patch.
Thanks again Pavel, David, Andy.
Thanks and Regards,
Ajay
On 11/6/21 22:20, Ajay Garg wrote:
> I vote for David's strscpy "fix", as it is simple, and does away with
> the dependency on the length of "func_table[kb_func]".
>
strscpy fix sounds reasonable to me. just to be save in future.
There is only one thing I am wondering about: translation table entries
are set by user using this struct
struct kbsentry {
unsigned char kb_func;
unsigned char kb_string[512];
};
it means entries cannot be longer than sizeof(kbsentry::kb_string) - 1
at all. Do we need extra branching with strscpy() or do we need to do
anything else here?
With regards,
Pavel Skripkin
Actually, on further thoughts, even David's solution will require an
extra check, if -E2BIG is returned.
So, I guess the solution suggested by me looks the best
(https://lore.kernel.org/linux-serial/[email protected]/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1)
:
1.
== Do not use the return value from strlcpy. ==
len = strlcpy(kbs, func_table[kb_func] ? : "", len);
=>
strlcpy(kbs, func_table[kb_func] ? : "", len);
2.
== Calculate the actual length of kbs, add 1, and then copy those many
bytes to user-buffer ==
ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
-EFAULT : 0;
=>
ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
-EFAULT : 0;
On Sun, Nov 7, 2021 at 12:50 AM Ajay Garg <[email protected]> wrote:
>
> Thanks Pavel, Andy, David for the help.
>
> Andy,
>
> There is no compilation/runtime blocker.
> There were warnings reported by smatch.
>
> My intention is to make the method "vt_do_kdgkb_ioctl" bullet-proof in
> itself, without depending upon external clients.
>
> Pavel has explained that currently things are fine, as per :
> https://lore.kernel.org/linux-serial/[email protected]/T/#m740fffb7c6ee52fdc98b9ef0b4e32a060b6a3be3
>
> but it seems that there is a big flaw - we are dependent on the length
> of "func_table[kb_func]" being ok. If func_table[kb_func] goes awry,
> the method will cause overflow.
>
> Since func_table[kb_func]" is not managed by the method, so the method
> must not depend on func_table[kb_func]" length-correctness. Instead,
> "vt_do_kdgkb_ioctl" must ensure no overflow, without depending how
> external entities (func_table[kb_func] behave.
>
>
>
> The issue with strlcpy, along with a potential "fix", has been explained in :
> https://lore.kernel.org/linux-serial/[email protected]/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1
>
> David has provided a simpler fix (usage of strscpy), as in :
> https://lore.kernel.org/linux-serial/[email protected]/T/#m63dab1137e593f2030920a53272f71866b442f40
>
>
> So, we could go with one of the above changes (mine/David's), or
> nothing at all (since there is no blocker).
>
> I vote for David's strscpy "fix", as it is simple, and does away with
> the dependency on the length of "func_table[kb_func]".
>
>
> Would like to know what the maintainers think.
> If there is a consensus that the method "vt_do_kdgkb_ioctl" be made
> bullet-proof in itself, please let me know, I will float the next
> version of patch.
>
>
> Thanks again Pavel, David, Andy.
>
>
> Thanks and Regards,
> Ajay
On Sun, Nov 7, 2021 at 1:26 AM Pavel Skripkin <[email protected]> wrote:
>
> On 11/6/21 22:20, Ajay Garg wrote:
> > I vote for David's strscpy "fix", as it is simple, and does away with
> > the dependency on the length of "func_table[kb_func]".
> >
>
> strscpy fix sounds reasonable to me. just to be save in future.
>
> There is only one thing I am wondering about: translation table entries
> are set by user using this struct
>
> struct kbsentry {
> unsigned char kb_func;
> unsigned char kb_string[512];
> };
>
> it means entries cannot be longer than sizeof(kbsentry::kb_string) - 1
> at all. Do we need extra branching with strscpy() or do we need to do
> anything else here?
Hi Pavel,
Please see my latest comments in the last reply.
>
>
>
> With regards,
> Pavel Skripkin
On 11/6/21 22:46, Ajay Garg wrote:
> Actually, on further thoughts, even David's solution will require an
> extra check, if -E2BIG is returned.
>
> So, I guess the solution suggested by me looks the best
> (https://lore.kernel.org/linux-serial/[email protected]/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1)
> :
>
> 1.
> == Do not use the return value from strlcpy. ==
>
> len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> =>
> strlcpy(kbs, func_table[kb_func] ? : "", len);
>
>
> 2.
> == Calculate the actual length of kbs, add 1, and then copy those many
> bytes to user-buffer ==
>
> ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
> -EFAULT : 0;
> =>
> ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
> -EFAULT : 0;
>
But isn't strlen(kbs) is guaranteed to be equal to strlcpy() return
value in this case? As I said in previous emails,
strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) by design of
this function.
Do we need extra strlen() call here? Let's see what more experienced
people think about it :)
With regards,
Pavel Skripkin
On 11/6/21 23:30, Ajay Garg wrote:
>> > 2.
>> > == Calculate the actual length of kbs, add 1, and then copy those many
>> > bytes to user-buffer ==
>> >
>> > ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
>> > -EFAULT : 0;
>> > =>
>> > ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
>> > -EFAULT : 0;
>> >
>>
>> But isn't strlen(kbs) is guaranteed to be equal to strlcpy() return
>> value in this case? As I said in previous emails,
>> strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) by design of
>> this function.
>
> That's the whole point of the discussion :)
>
> The method "vt_do_kdgkb_ioctl" does not manage "func_table[kb_func]".
> Thus, the method does not know whether or not
> strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string).
>
It manages. The code under `case KDSKBSENT:` sets func_table[] entries
via vt_kdskbsent().
kbs = strndup_user(..., sizeof(user_kdgkb->kb_string));
is used to allocate buffer for the func_table[] entry. That's my main
point :)
With regards,
Pavel Skripkin
> >
> > That's the whole point of the discussion :)
> >
> > The method "vt_do_kdgkb_ioctl" does not manage "func_table[kb_func]".
> > Thus, the method does not know whether or not
> > strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string).
> >
>
> It manages. The code under `case KDSKBSENT:` sets func_table[] entries
> via vt_kdskbsent().
>
> kbs = strndup_user(..., sizeof(user_kdgkb->kb_string));
>
> is used to allocate buffer for the func_table[] entry. That's my main
> point :)
func_table is set in vt_kdskbent, which itself is external.
More importantly, vt_kdskbent is handled in case KDSKBSENT:, while the
strlcpy issue we are dealing with is in case KDGKBSENT:
In case KDGKBSENT, following are managed :
ssize_t len = sizeof(user_kdgkb->kb_string);
kbs = kmalloc(len, GFP_KERNEL);
while func_table[kb_func] is external entity here, so no assumption
ought to be made for it, just my 2 cents though :)
Anyhow, really, it is the maintainers' choice now :), since there
isn't a burning (compilation/runtime) issue.
>
>
>
>
> With regards,
> Pavel Skripkin
On 11/6/21 23:44, Ajay Garg wrote:
>> >
>> > That's the whole point of the discussion :)
>> >
>> > The method "vt_do_kdgkb_ioctl" does not manage "func_table[kb_func]".
>> > Thus, the method does not know whether or not
>> > strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string).
>> >
>>
>> It manages. The code under `case KDSKBSENT:` sets func_table[] entries
>> via vt_kdskbsent().
>>
>> kbs = strndup_user(..., sizeof(user_kdgkb->kb_string));
>>
>> is used to allocate buffer for the func_table[] entry. That's my main
>> point :)
>
> func_table is set in vt_kdskbent, which itself is external.
>
> More importantly, vt_kdskbent is handled in case KDSKBSENT:, while the
> strlcpy issue we are dealing with is in case KDGKBSENT:
> In case KDGKBSENT, following are managed :
>
> ssize_t len = sizeof(user_kdgkb->kb_string);
> kbs = kmalloc(len, GFP_KERNEL);
>
> while func_table[kb_func] is external entity here, so no assumption
> ought to be made for it, just my 2 cents though :)
>
> Anyhow, really, it is the maintainers' choice now :), since there
> isn't a burning (compilation/runtime) issue.
>
I fully agree here, it's maintainer's choice. Let's sit down and wait
what experienced people thing about this :)
I've just wanted to explain my idea better to exclude possible
misunderstanding.
Thanks
With regards,
Pavel Skripkin
> > 2.
> > == Calculate the actual length of kbs, add 1, and then copy those many
> > bytes to user-buffer ==
> >
> > ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
> > -EFAULT : 0;
> > =>
> > ret = copy_to_user(user_kdgkb->kb_string, kbs, strlen(kbs) + 1) ?
> > -EFAULT : 0;
> >
>
> But isn't strlen(kbs) is guaranteed to be equal to strlcpy() return
> value in this case? As I said in previous emails,
> strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) by design of
> this function.
That's the whole point of the discussion :)
The method "vt_do_kdgkb_ioctl" does not manage "func_table[kb_func]".
Thus, the method does not know whether or not
strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string).
The intention is to make the method itself robust, without relying on
any external "chances" :)
>
> Do we need extra strlen() call here? Let's see what more experienced
> people think about it :)
Yep, let's wait for more feedback ..
Dropping all further discussions on this thread, as a RFC for a new
string-copy method has been posted at :
https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#t
which, if accepted, will make the clients' lives a lot easier.
Hi Pavel,
>
> Honestly, I can't get what you are trying to achieve with new string
> function.
>
> If caller knows, that there is no possible overflow, it can omit bounds
> checking (like in vt_do_kdgkb_ioctl). If caller needs return value equal
> to destination length it can use strscpy().
Please see the output corresponding for strscpy(), in the example output at
https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#m4a3f524eefe283a42430905fa4c0dfc2c37b2819
As is evident, even though destination length is 9, yet the returned
value is -7 (corresponding to -E2BIG).
So, strscpy() fails.
>
> There is a bunch of str*cpy() functions and every month I see new
> conversations between them on ML. As Andy said it's really chaos. These
> conversation are needed, of course, from security point of view, but
> lib/string is already big. It contains functions for every possible
> scenario, caller just needs to pick right one.
lib/string is big or small, that's not an excuse imho :)
I care about simplicity and easy lives for everyone in present (of
course) and future (more importantly).
As mentioned in :
https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#m4a3f524eefe283a42430905fa4c0dfc2c37b2819
there are several cases in files like fs/kernfs/dir.c, where
strlcpy()'s return value is directly propogated to the clients, and it
is not evident whether or not the return-value is within bounds.
If the new intended method is not added, we need to add checks in all
the clients (which is too much churn).
Instead, the new intended method will simplify lives for the clients,
when all they care is copy as much bytes as possible, and get the
number of bytes actualy copied.
It would be beneficial for all if discussions about the new method are
done on the intended thread.
Thanks and Regards,
Ajay
On 11/8/21 11:59, Ajay Garg wrote:
> Dropping all further discussions on this thread, as a RFC for a new
> string-copy method has been posted at :
> https://lore.kernel.org/linux-hardening/CAHP4M8U=0aTHgfREGJpSboV6J4X+E3Y6+H_kb-PvXxDKtV=n-g@mail.gmail.com/T/#t
>
> which, if accepted, will make the clients' lives a lot easier.
>
Honestly, I can't get what you are trying to achieve with new string
function.
If caller knows, that there is no possible overflow, it can omit bounds
checking (like in vt_do_kdgkb_ioctl). If caller needs return value equal
to destination length it can use strscpy().
There is a bunch of str*cpy() functions and every month I see new
conversations between them on ML. As Andy said it's really chaos. These
conversation are needed, of course, from security point of view, but
lib/string is already big. It contains functions for every possible
scenario, caller just needs to pick right one.
I might be too dumb in this topic, so it's just my IMHO, since I am on
CC list.
With regards,
Pavel Skripkin
On 06. 11. 21, 21:48, Pavel Skripkin wrote:
> On 11/6/21 23:44, Ajay Garg wrote:
>>> >
>>> > That's the whole point of the discussion :)
>>> >
>>> > The method "vt_do_kdgkb_ioctl" does not manage "func_table[kb_func]".
>>> > Thus, the method does not know whether or not
>>> > strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string).
>>> >
>>>
>>> It manages. The code under `case KDSKBSENT:` sets func_table[] entries
>>> via vt_kdskbsent().
>>>
>>> kbs = strndup_user(..., sizeof(user_kdgkb->kb_string));
>>>
>>> is used to allocate buffer for the func_table[] entry. That's my main
>>> point :)
>>
>> func_table is set in vt_kdskbent, which itself is external.
>>
>> More importantly, vt_kdskbent is handled in case KDSKBSENT:, while the
>> strlcpy issue we are dealing with is in case KDGKBSENT:
>> In case KDGKBSENT, following are managed :
>>
>> ssize_t len = sizeof(user_kdgkb->kb_string);
>> kbs = kmalloc(len, GFP_KERNEL);
>>
>> while func_table[kb_func] is external entity here, so no assumption
>> ought to be made for it, just my 2 cents though :)
>>
>> Anyhow, really, it is the maintainers' choice now :), since there
>> isn't a burning (compilation/runtime) issue.
>>
>
> I fully agree here, it's maintainer's choice. Let's sit down and wait
> what experienced people thing about this :)
I don't quite understand what the problem is. Provided I wrote the code,
is there something wrong with this commit (and its explanation), in
particular?
commit 6ca03f90527e499dd5e32d6522909e2ad390896b
Author: Jiri Slaby <[email protected]>
Date: Mon Oct 19 10:55:16 2020 +0200
vt: keyboard, simplify vt_kdgkbsent
Use 'strlen' of the string, add one for NUL terminator and simply do
'copy_to_user' instead of the explicit 'for' loop. This makes the
KDGKBSENT case more compact.
The only thing we need to take care about is NULL 'func_table[i]'. Use
an empty string in that case.
The original check for overflow could never trigger as the func_buf
strings are always shorter or equal to 'struct kbsentry's.
thanks,
--
js
suse labs
On 11/10/21 08:22, Jiri Slaby wrote:
> I don't quite understand what the problem is. Provided I wrote the code,
> is there something wrong with this commit (and its explanation), in
> particular?
>
> commit 6ca03f90527e499dd5e32d6522909e2ad390896b
> Author: Jiri Slaby <[email protected]>
> Date: Mon Oct 19 10:55:16 2020 +0200
>
> vt: keyboard, simplify vt_kdgkbsent
>
> Use 'strlen' of the string, add one for NUL terminator and simply do
> 'copy_to_user' instead of the explicit 'for' loop. This makes the
> KDGKBSENT case more compact.
>
> The only thing we need to take care about is NULL 'func_table[i]'. Use
> an empty string in that case.
>
> The original check for overflow could never trigger as the func_buf
> strings are always shorter or equal to 'struct kbsentry's.
>
> thanks,
>
As I said in my few previous emails, I don't see any bugs/problems in
current code.
Ajay wants to be safe and he thinks, that relying on fact, that
strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) is not good
approach, since it's external for vt_do_kdgkb_ioctl. (I hope, I've
explained his idea in the right way)
With regards,
Pavel Skripkin
>
> Ajay wants to be safe and he thinks, that relying on fact, that
> strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) is not good
> approach, since it's external for vt_do_kdgkb_ioctl. (I hope, I've
> explained his idea in the right way)
>
That's right Pavel.
Every function must work correctly as it "advertises", instead of
relying on "chancy correctness" of the calls leading to the method.
On Wed, Nov 10, 2021 at 02:27:36PM +0530, Ajay Garg wrote:
> >
> > Ajay wants to be safe and he thinks, that relying on fact, that
> > strlen(func_table[kb_func]) < sizeof(user_kdgkb->kb_string) is not good
> > approach, since it's external for vt_do_kdgkb_ioctl. (I hope, I've
> > explained his idea in the right way)
> >
>
> That's right Pavel.
> Every function must work correctly as it "advertises", instead of
> relying on "chancy correctness" of the calls leading to the method.
That is not how the kernel works, sorry. Otherwise every function would
have to always verify all parameters passed to them, causing slow downs
and redundant checks everywhere.
When all users of functions are in the kernel tree itself, you can use
tools and manual verification that the code is correct, because those
are the only users of the functions.
thanks,
greg k-h
> >
> > That's right Pavel.
> > Every function must work correctly as it "advertises", instead of
> > relying on "chancy correctness" of the calls leading to the method.
>
> That is not how the kernel works, sorry. Otherwise every function would
> have to always verify all parameters passed to them, causing slow downs
> and redundant checks everywhere.
>
Hmm, agreed. Every cycle saved in the kernel is performance gained.
That's why, the RFC for strlscpy [1] makes all the more sense, as it
would save cpu cycles by removing the requirement to check the
return-value for overflows/underflows (including the "issue" I am
trying to address in this particular thread, and which actually lead
to the RFC for strlscpy].
P.S. :
I am not an egoistic person, who wants to get into unnecessary fights
just to upheld one's ego.
All I am trying is to suggest improvements, that
* make things faster.
* keeps code to as minimum as possible.
* makes developers' lives as comfortable as possible.
[1]
https://lore.kernel.org/linux-hardening/CAHP4M8WnLA0780yN+bpuuCtir+DLJRxe0atAiLbZO0bTGf6J-Q@mail.gmail.com/T/#m4a3f524eefe283a42430905fa4c0dfc2c37b2819
Thanks and Regards,
Ajay