Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Tue, 9 May 2023 15:24:37 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Azeem Shaikh <azeemshaikh38@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        security@kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] kernfs: Prefer strscpy over strlcpy calls
Message-ID: <ZFryVUHtkN5QbKTP@slm.duckdns.org>
References: <20230510011122.3040915-1-azeemshaikh38@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230510011122.3040915-1-azeemshaikh38@gmail.com>
Precedence: bulk

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