[PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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 <ajaygargnsit@gmail.com>
---
 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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Pavel Skripkin]

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 <ajaygargnsit@gmail.com>
> ---
>   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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[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

RE: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[David Laight]

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 <ajaygargnsit@gmail.com>
> > ---
> >   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)

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1

David has provided a simpler fix (usage of strscpy), as in :
https://lore.kernel.org/linux-serial/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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 <ajaygargnsit@gmail.com> 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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/T/#m1c4aaa4347b02fd4c11ce611ff5029fcb71c37a1
>
> David has provided a simpler fix (usage of strscpy), as in :
> https://lore.kernel.org/linux-serial/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[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/868025b485b94480ad17d0ec971b3ee9@AcuMS.aculab.com/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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

> > 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 ..

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

> >
> > 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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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.

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Pavel Skripkin]

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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Jiri Slaby]

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 <jirislaby@kernel.org>
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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Pavel Skripkin]

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 <jirislaby@kernel.org>
> 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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

>
> 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.

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Greg KH]

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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

> >
> > 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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Pavel Skripkin]

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

Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in copy_to_user

[Ajay Garg]

On Sun, Nov 7, 2021 at 1:26 AM Pavel Skripkin <paskripkin@gmail.com> 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