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;
Subject: Re: [PATCH 17/20] vt: Manual replacement of the deprecated strlcpy()
 with return values
To:     Romain Perier <romain.perier@gmail.com>,
        Kees Cook <keescook@chromium.org>,
        kernel-hardening@lists.openwall.com,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org
References: <20210222151231.22572-1-romain.perier@gmail.com>
 <20210222151231.22572-18-romain.perier@gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Message-ID: <a9f26339-c366-40c4-1cd6-c7ae1838e2b6@kernel.org>
Date:   Fri, 26 Feb 2021 10:49:02 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210222151231.22572-18-romain.perier@gmail.com>
Content-Type: text/plain; charset=iso-8859-2; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 22. 02. 21, 16:12, Romain Perier wrote:
> The strlcpy() reads the entire source buffer first, it is dangerous if
> the source buffer lenght is unbounded or possibility non NULL-terminated.

"length" and it's NUL, not NULL in this case.

> It can lead to linear read overflows, crashes, etc...
> 
> As recommended in the deprecated interfaces [1], it should be replaced
> by strscpy.
> 
> This commit replaces all calls to strlcpy that handle the return values

s/that/which/ ?
"handles"
"value"

> by the corresponding strscpy calls with new handling of the return
> values (as it is quite different between the two functions).

Sorry, I have hard times understand the whole sentence. Could you 
rephrase a bit?

> [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy
> 
> Signed-off-by: Romain Perier <romain.perier@gmail.com>
> ---
>   drivers/tty/vt/keyboard.c |    5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 77638629c562..5e20c6c307e0 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -2067,9 +2067,12 @@ 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);

func_table[kb_func] is NUL-terminated and kbs is of length len anyway, 
so this is only cosmetical.

>   		spin_unlock_irqrestore(&func_buf_lock, flags);
>   
> +		if (len == -E2BIG)
> +			return -E2BIG;
> +

This can never happen, right?

>   		ret = copy_to_user(user_kdgkb->kb_string, kbs, len + 1) ?
>   			-EFAULT : 0;
>   
> 

thanks,
-- 
js