Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <3d5e87ad-384a-f024-6b4e-8439c983cbfc@gmail.com>
Date:   Sat, 6 Nov 2021 15:39:12 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH] tty: vt: keyboard: do not copy an extra-byte in
 copy_to_user
Content-Language: en-US
To:     Ajay Garg <ajaygargnsit@gmail.com>
Cc:     linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20211106092041.43745-1-ajaygargnsit@gmail.com>
 <9eafae1f-d9f0-298d-cf20-212865d0becc@gmail.com>
 <CAHP4M8X5ZrrVBS6Y3tg6c8jK4BA0JK+q8reiGwVeoZX9gUvogw@mail.gmail.com>
From:   Pavel Skripkin <paskripkin@gmail.com>
In-Reply-To: <CAHP4M8X5ZrrVBS6Y3tg6c8jK4BA0JK+q8reiGwVeoZX9gUvogw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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