linux-kernel - Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZFryVUHtkN5QbKTP@slm.duckdns.org>
Date:   Tue, 9 May 2023 15:24:37 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Azeem Shaikh <azeemshaikh38@...il.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        security@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls

On Wed, May 10, 2023 at 01:11:22AM +0000, Azeem Shaikh wrote:
...
> +/* strscpy_mock_strlcpy - imitates strlcpy API but uses strscpy underneath. */
> +static size_t strscpy_mock_strlcpy(char *dest, const char *src, size_t count)
> +{
> +	strscpy(dest, src, count);
> +	return strlen(src);
> +}

I'm not sure this is a meaningful conversion. One benefit of strscpy() is
that it's less error-prone to check for overflows. What's the point of
removing strlcpy() if we end up sprinkling lesser duplicates in the tree?

>  static int kernfs_name_locked(struct kernfs_node *kn, char *buf, size_t buflen)
>  {
>  	if (!kn)
> -		return strlcpy(buf, "(null)", buflen);
> +		return strscpy_mock_strlcpy(buf, "(null)", buflen);
>  
> -	return strlcpy(buf, kn->parent ? kn->name : "/", buflen);
> +	return strscpy_mock_strlcpy(buf, kn->parent ? kn->name : "/", buflen);
>  }

Can you follow the users and convert the users accordingly rather than
masking it from here? ie. make kernfs_name() and friends return -E2BIG when
source is too long like strscpy(). I don't think anybody cares, actually.

>  /* kernfs_node_depth - compute depth from @from to @to */
> @@ -141,13 +148,13 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>  	int i, j;
>  
>  	if (!kn_to)
> -		return strlcpy(buf, "(null)", buflen);
> +		return strscpy_mock_strlcpy(buf, "(null)", buflen);
>  
>  	if (!kn_from)
>  		kn_from = kernfs_root(kn_to)->kn;
>  
>  	if (kn_from == kn_to)
> -		return strlcpy(buf, "/", buflen);
> +		return strscpy_mock_strlcpy(buf, "/", buflen);
>  
>  	common = kernfs_common_ancestor(kn_from, kn_to);
>  	if (WARN_ON(!common))
> @@ -159,16 +166,16 @@ static int kernfs_path_from_node_locked(struct kernfs_node *kn_to,
>  	buf[0] = '\0';
>  
>  	for (i = 0; i < depth_from; i++)
> -		len += strlcpy(buf + len, parent_str,
> +		len += strscpy_mock_strlcpy(buf + len, parent_str,
>  			       len < buflen ? buflen - len : 0);
>  
>  	/* Calculate how many bytes we need for the rest */
>  	for (i = depth_to - 1; i >= 0; i--) {
>  		for (kn = kn_to, j = 0; j < i; j++)
>  			kn = kn->parent;
> -		len += strlcpy(buf + len, "/",
> +		len += strscpy_mock_strlcpy(buf + len, "/",
>  			       len < buflen ? buflen - len : 0);
> -		len += strlcpy(buf + len, kn->name,
> +		len += strscpy_mock_strlcpy(buf + len, kn->name,
>  			       len < buflen ? buflen - len : 0);
>  	}

Ditto, please convert all the users accordingly. If that's not feasible, I
think it'd be better to leave it as-is. I don't see how the new code is
better.

> @@ -851,7 +858,7 @@ static struct kernfs_node *kernfs_walk_ns(struct kernfs_node *parent,
>  
>  	spin_lock_irq(&kernfs_pr_cont_lock);
>  
> -	len = strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
> +	len = strscpy_mock_strlcpy(kernfs_pr_cont_buf, path, sizeof(kernfs_pr_cont_buf));
>  
>  	if (len >= sizeof(kernfs_pr_cont_buf)) {
>  		spin_unlock_irq(&kernfs_pr_cont_lock);

This is an easy conversion to strscpy().

Thanks.

-- 
tejun