strlcpy() reads the entire source buffer first.
This read may exceed the destination size limit if
a source string is not NUL-terminated [1].

The copy_to_user() call uses @len returned from strlcpy() directly
without checking its value. This could potentially lead to read
overflow.

In an effort to remove strlcpy() completely [2], replace
strlcpy() here with strscpy().

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
[2] https://github.com/KSPP/linux/issues/89

Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
---
 drivers/tty/vt/keyboard.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 358f216c6cd6..15359c328a23 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -2079,12 +2079,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
 			return -ENOMEM;

 		spin_lock_irqsave(&func_buf_lock, flags);
-		len = strlcpy(kbs, func_table[kb_func] ? : "", len);
+		len = strscpy(kbs, func_table[kb_func] ? : "", len);
 		spin_unlock_irqrestore(&func_buf_lock, flags);

+		if (len < 0) {
+			ret = -EFAULT;
+			break;
+		}
 		ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
 			-EFAULT : 0;
-
 		break;
 	}
 	case KDSKBSENT:
--
2.42.0.rc2.253.gd59a3bf2b4-goog

On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.

Perhaps add:
"... and returns the size of the source string, not the destination
string, which can be accidentally misused."

> This read may exceed the destination size limit if
> a source string is not NUL-terminated [1].
> 
> The copy_to_user() call uses @len returned from strlcpy() directly
> without checking its value. This could potentially lead to read
> overflow.

Since the code as written today is "accidentally correct", it's worth
clarifying this: there is no existing bug, but as written it is very
fragile and specifically uses a strlcpy() result without sanity checking
and using it to copy to userspace. (This is the exact anti-pattern for
strlcpy(), and only because the source strings are known good is this
safe.)

> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
>  drivers/tty/vt/keyboard.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 358f216c6cd6..15359c328a23 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2079,12 +2079,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  			return -ENOMEM;
> 
>  		spin_lock_irqsave(&func_buf_lock, flags);
> -		len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> +		len = strscpy(kbs, func_table[kb_func] ? : "", len);
>  		spin_unlock_irqrestore(&func_buf_lock, flags);
> 
> +		if (len < 0) {
> +			ret = -EFAULT;

I think this should be -ENOSPC as EFAULT implies an actual memory fault.

> +			break;
> +		}
>  		ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
>  			-EFAULT : 0;
> -
>  		break;
>  	}
>  	case KDSKBSENT:
> --
> 2.42.0.rc2.253.gd59a3bf2b4-goog
> 
> 

Thanks for sticking with these refactorings; we're almost free from
strlcpy. :)

-Kees

-- 
Kees Cook

On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> strlcpy() reads the entire source buffer first.
> This read may exceed the destination size limit if
> a source string is not NUL-terminated [1].

But that's not the case here, right?  So your "potential read overflow"
isn't relevant here.

> The copy_to_user() call uses @len returned from strlcpy() directly
> without checking its value. This could potentially lead to read
> overflow.

But can it?  How?

Those are all hard-coded strings, in the kernel source, there is no
potential overflow here.

And you know the buffer size is correct as well.

So why even check?

> In an effort to remove strlcpy() completely [2], replace
> strlcpy() here with strscpy().
> 
> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> [2] https://github.com/KSPP/linux/issues/89
> 
> Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com>
> ---
>  drivers/tty/vt/keyboard.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 358f216c6cd6..15359c328a23 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2079,12 +2079,15 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm)
>  			return -ENOMEM;
> 
>  		spin_lock_irqsave(&func_buf_lock, flags);
> -		len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> +		len = strscpy(kbs, func_table[kb_func] ? : "", len);
>  		spin_unlock_irqrestore(&func_buf_lock, flags);
> 
> +		if (len < 0) {
> +			ret = -EFAULT;
> +			break;
> +		}

Don't check for impossible things please.

thanks,

greg k-h

On Wed, Aug 30, 2023 at 1:57 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> > strlcpy() reads the entire source buffer first.
> > This read may exceed the destination size limit if
> > a source string is not NUL-terminated [1].
>
> But that's not the case here, right?  So your "potential read overflow"
> isn't relevant here.
>
> > The copy_to_user() call uses @len returned from strlcpy() directly
> > without checking its value. This could potentially lead to read
> > overflow.
>
> But can it?  How?
>

The case I was considering is when the null-terminated hardcoded
string @func_table[kb_func] has length @new_len > @len. In this case,
strlcpy() will assign @len = @new_len and copy_to_user() would read
@new_len from the kmalloc-ed memory of @len. This is the potential
read overflow I was referring to. Let me know if I'm mistaken.

On Wed, Aug 30, 2023 at 03:25:54PM -0400, Azeem Shaikh wrote:
> On Wed, Aug 30, 2023 at 1:57 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> > > strlcpy() reads the entire source buffer first.
> > > This read may exceed the destination size limit if
> > > a source string is not NUL-terminated [1].
> >
> > But that's not the case here, right?  So your "potential read overflow"
> > isn't relevant here.
> >
> > > The copy_to_user() call uses @len returned from strlcpy() directly
> > > without checking its value. This could potentially lead to read
> > > overflow.
> >
> > But can it?  How?
> >
> 
> The case I was considering is when the null-terminated hardcoded
> string @func_table[kb_func] has length @new_len > @len. In this case,
> strlcpy() will assign @len = @new_len and copy_to_user() would read
> @new_len from the kmalloc-ed memory of @len. This is the potential
> read overflow I was referring to. Let me know if I'm mistaken.

First there is:

ssize_t len = sizeof(user_kdgkb->kb_string);

"struct user_kdgkb" is UAPI (therefore unlikely to change), and kb_string
is 512:

struct kbsentry {
        unsigned char kb_func;
        unsigned char kb_string[512];
};

Then we do:

                len = strlcpy(kbs, func_table[kb_func] ? : "", len);

This is the anti-pattern (take the length of the _source_) we need to
remove. However, func_table[] is made up of either %NUL-terminated
strings:

char func_buf[] = {
        '\033', '[', '[', 'A', 0, 
        '\033', '[', '[', 'B', 0, 
...
char *func_table[MAX_NR_FUNC] = {
        func_buf + 0,
        func_buf + 5,
...

Or a NULL address itself. The ?: operator handles the NULL case, so
"len" can only ever be 0 through the longest string in func_buf. So it's
what I'd call "accidentally correct". i.e. it's using a fragile
anti-pattern, but in this case everything is hard-coded and less than
512.

Regardless, we need to swap for a sane pattern, which you've done. But
the commit log is misleading, so it needs some more detail. :)

-Kees

-- 
Kees Cook

On 30. 08. 23, 23:28, Kees Cook wrote:
> On Wed, Aug 30, 2023 at 03:25:54PM -0400, Azeem Shaikh wrote:
>> On Wed, Aug 30, 2023 at 1:57 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>>
>>> On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
>>>> strlcpy() reads the entire source buffer first.
>>>> This read may exceed the destination size limit if
>>>> a source string is not NUL-terminated [1].
>>>
>>> But that's not the case here, right?  So your "potential read overflow"
>>> isn't relevant here.
>>>
>>>> The copy_to_user() call uses @len returned from strlcpy() directly
>>>> without checking its value. This could potentially lead to read
>>>> overflow.
>>>
>>> But can it?  How?
>>>
>>
>> The case I was considering is when the null-terminated hardcoded
>> string @func_table[kb_func] has length @new_len > @len. In this case,
>> strlcpy() will assign @len = @new_len and copy_to_user() would read
>> @new_len from the kmalloc-ed memory of @len. This is the potential
>> read overflow I was referring to. Let me know if I'm mistaken.
> 
> First there is:
> 
> ssize_t len = sizeof(user_kdgkb->kb_string);
> 
> "struct user_kdgkb" is UAPI (therefore unlikely to change), and kb_string
> is 512:
> 
> struct kbsentry {
>          unsigned char kb_func;
>          unsigned char kb_string[512];
> };
> 
> Then we do:
> 
>                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> 
> This is the anti-pattern (take the length of the _source_) we need to
> remove.

But len is the length of kbs, i.e. the destination. Or what am I missing?

                 kbs = kmalloc(len, GFP_KERNEL);
                 len = strlcpy(kbs, func_table[kb_func] ? : "", len);

> However, func_table[] is made up of either %NUL-terminated
> strings:
> 
> char func_buf[] = {
>          '\033', '[', '[', 'A', 0,
>          '\033', '[', '[', 'B', 0,
> ...
> char *func_table[MAX_NR_FUNC] = {
>          func_buf + 0,
>          func_buf + 5,
> ...
> 
> Or a NULL address itself. The ?: operator handles the NULL case, so
> "len" can only ever be 0 through the longest string in func_buf. So it's
> what I'd call "accidentally correct". i.e. it's using a fragile
> anti-pattern, but in this case everything is hard-coded and less than
> 512.
> 
> Regardless, we need to swap for a sane pattern, which you've done. But
> the commit log is misleading, so it needs some more detail. :)

I am still missing what is wrong in the above code with strlcpy(). The 
dest is deliberately made as long as the source, so the returned len is 
never less then the passed len. No checking needed IMO. Perhaps, we 
might switch to strcpy()?

FWIW I introduced this in commit 82e61c3909db5. So if it needs fixing, 
the patch deserves a Fixes: 82e61c3909db5 tag.

thanks,
-- 
js
suse labs

On Thu, Aug 31, 2023 at 07:32:18AM +0200, Jiri Slaby wrote:
> On 30. 08. 23, 23:28, Kees Cook wrote:
> >                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> > 
> > This is the anti-pattern (take the length of the _source_) we need to
> > remove.
> 
> But len is the length of kbs, i.e. the destination. Or what am I missing?

strlcpy() returns the length of the _source_ string (i.e. it could be
greater than the input argument len). But there is no current flaw here
(since all sources are very short). We're just trying to remove strlcpy()
since it leads to unexpected results.

-Kees

-- 
Kees Cook

On Thu, Aug 31, 2023 at 1:32 AM Jiri Slaby <jirislaby@kernel.org> wrote:
>
> On 30. 08. 23, 23:28, Kees Cook wrote:
> > On Wed, Aug 30, 2023 at 03:25:54PM -0400, Azeem Shaikh wrote:
> >> On Wed, Aug 30, 2023 at 1:57 PM Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >>>
> >>> On Wed, Aug 30, 2023 at 04:04:10PM +0000, Azeem Shaikh wrote:
> >>>> strlcpy() reads the entire source buffer first.
> >>>> This read may exceed the destination size limit if
> >>>> a source string is not NUL-terminated [1].
> >>>
> >>> But that's not the case here, right?  So your "potential read overflow"
> >>> isn't relevant here.
> >>>
> >>>> The copy_to_user() call uses @len returned from strlcpy() directly
> >>>> without checking its value. This could potentially lead to read
> >>>> overflow.
> >>>
> >>> But can it?  How?
> >>>
> >>
> >> The case I was considering is when the null-terminated hardcoded
> >> string @func_table[kb_func] has length @new_len > @len. In this case,
> >> strlcpy() will assign @len = @new_len and copy_to_user() would read
> >> @new_len from the kmalloc-ed memory of @len. This is the potential
> >> read overflow I was referring to. Let me know if I'm mistaken.
> >
> > First there is:
> >
> > ssize_t len = sizeof(user_kdgkb->kb_string);
> >
> > "struct user_kdgkb" is UAPI (therefore unlikely to change), and kb_string
> > is 512:
> >
> > struct kbsentry {
> >          unsigned char kb_func;
> >          unsigned char kb_string[512];
> > };
> >
> > Then we do:
> >
> >                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
> >
> > This is the anti-pattern (take the length of the _source_) we need to
> > remove.
>
> But len is the length of kbs, i.e. the destination. Or what am I missing?
>
>                  kbs = kmalloc(len, GFP_KERNEL);
>                  len = strlcpy(kbs, func_table[kb_func] ? : "", len);
>
> > However, func_table[] is made up of either %NUL-terminated
> > strings:
> >
> > char func_buf[] = {
> >          '\033', '[', '[', 'A', 0,
> >          '\033', '[', '[', 'B', 0,
> > ...
> > char *func_table[MAX_NR_FUNC] = {
> >          func_buf + 0,
> >          func_buf + 5,
> > ...
> >
> > Or a NULL address itself. The ?: operator handles the NULL case, so
> > "len" can only ever be 0 through the longest string in func_buf. So it's
> > what I'd call "accidentally correct". i.e. it's using a fragile
> > anti-pattern, but in this case everything is hard-coded and less than
> > 512.
> >
> > Regardless, we need to swap for a sane pattern, which you've done. But
> > the commit log is misleading, so it needs some more detail. :)
>
> I am still missing what is wrong in the above code with strlcpy(). The
> dest is deliberately made as long as the source, so the returned len is
> never less then the passed len. No checking needed IMO. Perhaps, we
> might switch to strcpy()?
>
> FWIW I introduced this in commit 82e61c3909db5. So if it needs fixing,
> the patch deserves a Fixes: 82e61c3909db5 tag.
>

As explained by Kees in previous comments, this is not actually a bug,
just a fragile anti-pattern. So we shouldn't need the Fixes: tag on
this patch.

> thanks,
> --
> js
> suse labs
>

>>>> The copy_to_user() call uses @len returned from strlcpy() directly
>>>> without checking its value. This could potentially lead to read
>>>> overflow.
>>>
>>> But can it?  How?
>>>
>>
>> The case I was considering is when the null-terminated hardcoded
>> string @func_table[kb_func] has length @new_len > @len. In this case,
>> strlcpy() will assign @len = @new_len and copy_to_user() would read
>> @new_len from the kmalloc-ed memory of @len. This is the potential
>> read overflow I was referring to. Let me know if I'm mistaken.
> 
> ..it's what I'd call "accidentally correct". i.e. it's using a fragile> anti-pattern, but in this case everything is hard-coded and less than
> 512.
> 
> Regardless, we need to swap for a sane pattern, which you've done. But
> the commit log is misleading, so it needs some more detail. :)
> 
> -Kees

In my opinion strlcpy() is being used correctly here as a defensive
precaution.  If the source string is larger than the destination buffer
it will truncate rather than corrupt kernel memory.  However the
return value of strlcpy() is being misused.  If truncation occurred
the copy_to_user() call will corrupt user memory instead.

I also agree that this is not currently a bug.  It is fragile and it
could break if someone added a very large string to the table.

Why not fix this by avoiding the redundant string copy?  How about
something like this:

ptr = func_table[kb_func] ? : "";
len = strlen(ptr);

if (len >= sizeof(user_kdgkb->kb_string))
	return -ENOSPC;

if (copy_to_user(user_kdgkb->kb_string, ptr, len + 1))
	return -EFAULT;

On Wed, Aug 30, 2023 at 05:17:12PM -0600, Dan Raymond wrote:
> In my opinion strlcpy() is being used correctly here as a defensive
> precaution.  If the source string is larger than the destination buffer
> it will truncate rather than corrupt kernel memory.  However the
> return value of strlcpy() is being misused.  If truncation occurred
> the copy_to_user() call will corrupt user memory instead.
> 
> I also agree that this is not currently a bug.  It is fragile and it
> could break if someone added a very large string to the table.
> 
> Why not fix this by avoiding the redundant string copy?  How about
> something like this:
> 
> ptr = func_table[kb_func] ? : "";
> len = strlen(ptr);
> 
> if (len >= sizeof(user_kdgkb->kb_string))
> 	return -ENOSPC;
> 
> if (copy_to_user(user_kdgkb->kb_string, ptr, len + 1))
> 	return -EFAULT;

This would work if not for func_buf_lock. The bounce buffer is used to
avoid needing to hold the spin lock across copy_to_user.

-- 
Kees Cook

On 8/30/2023 5:48 PM, Kees Cook wrote:
> Warning: This email is from an unusual correspondent.
> Warning: Make sure this is someone you trust.
> 
> On Wed, Aug 30, 2023 at 05:17:12PM -0600, Dan Raymond wrote:
>> In my opinion strlcpy() is being used correctly here as a defensive
>> precaution.  If the source string is larger than the destination buffer
>> it will truncate rather than corrupt kernel memory.  However the
>> return value of strlcpy() is being misused.  If truncation occurred
>> the copy_to_user() call will corrupt user memory instead.
>>
>> I also agree that this is not currently a bug.  It is fragile and it
>> could break if someone added a very large string to the table.
>>
>> Why not fix this by avoiding the redundant string copy?  How about
>> something like this:
>>
>> ptr = func_table[kb_func] ? : "";
>> len = strlen(ptr);
>>
>> if (len >= sizeof(user_kdgkb->kb_string))
>> 	return -ENOSPC;
>>
>> if (copy_to_user(user_kdgkb->kb_string, ptr, len + 1))
>> 	return -EFAULT;
> 
> This would work if not for func_buf_lock. The bounce buffer is used to
> avoid needing to hold the spin lock across copy_to_user.
> 

Ah you're right.  Thanks for setting me straight.  Now I realize that my
entire assessment was wrong.  The original author was not using strlcpy()
as a defensive measure to prevent a buffer overflow.  He was using it so
that he could create a copy of the string and measure its length using
only one pass.  This minimizes the time spent holding the spinlock.

The surrounding code was written such that a buffer overflow is
impossible.  No additional checks are needed.  The proposed patch is
unnecessary.  But at least it preserves the spirit of the original
author's code by performing only one pass of the source string
while holding the spinlock.

On Thu, Aug 31, 2023 at 1:45 AM Dan Raymond <draymond@foxvalley.net> wrote:
>
> On 8/30/2023 5:48 PM, Kees Cook wrote:
> > Warning: This email is from an unusual correspondent.
> > Warning: Make sure this is someone you trust.
> >
> > On Wed, Aug 30, 2023 at 05:17:12PM -0600, Dan Raymond wrote:
> >> In my opinion strlcpy() is being used correctly here as a defensive
> >> precaution.  If the source string is larger than the destination buffer
> >> it will truncate rather than corrupt kernel memory.  However the
> >> return value of strlcpy() is being misused.  If truncation occurred
> >> the copy_to_user() call will corrupt user memory instead.
> >>
> >> I also agree that this is not currently a bug.  It is fragile and it
> >> could break if someone added a very large string to the table.
> >>
> >> Why not fix this by avoiding the redundant string copy?  How about
> >> something like this:
> >>
> >> ptr = func_table[kb_func] ? : "";
> >> len = strlen(ptr);
> >>
> >> if (len >= sizeof(user_kdgkb->kb_string))
> >>      return -ENOSPC;
> >>
> >> if (copy_to_user(user_kdgkb->kb_string, ptr, len + 1))
> >>      return -EFAULT;
> >
> > This would work if not for func_buf_lock. The bounce buffer is used to
> > avoid needing to hold the spin lock across copy_to_user.
> >
>
> Ah you're right.  Thanks for setting me straight.  Now I realize that my
> entire assessment was wrong.  The original author was not using strlcpy()
> as a defensive measure to prevent a buffer overflow.  He was using it so
> that he could create a copy of the string and measure its length using
> only one pass.  This minimizes the time spent holding the spinlock.
>
> The surrounding code was written such that a buffer overflow is
> impossible.  No additional checks are needed.  The proposed patch is
> unnecessary.  But at least it preserves the spirit of the original
> author's code by performing only one pass of the source string
> while holding the spinlock.

Are folks ok with me sending out a v2 for this with a better commit
log that explains the issue?

On Thu, Aug 31, 2023 at 10:23:10AM -0400, Azeem Shaikh wrote:
> Are folks ok with me sending out a v2 for this with a better commit
> log that explains the issue?

Yes, please do. It should clear up the questions from this thread. :)

Thanks!

-Kees

-- 
Kees Cook