netdev - Re: [PATCH] net: rds transform strncpy to strscpy

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250408182203.GH395307@horms.kernel.org>
Date: Tue, 8 Apr 2025 19:22:03 +0100
From: Simon Horman <horms@...nel.org>
To: Baris Can Goral <goralbaris@...il.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, netdev@...r.kernel.org,
	allison.henderson@...cle.com, skhan@...uxfoundation.org,
	linux-rdma@...r.kernel.org
Subject: Re: [PATCH] net: rds transform strncpy to strscpy

+ linux-rdma@...r.kernel.org

Hi Baris,

On Mon, Apr 07, 2025 at 09:30:53PM +0300, Baris Can Goral wrote:
> Hi,

It's nice to be friendly, but I don't think a salutation
belongs in a commit message. (Please remove the line above.)

> The strncpy() function is actively dangerous to use since it may not
> NULL-terminate the destination string,resulting in potential memory

Space after the comma (,) please.

> content exposures, unbounded reads, or crashes.

I think there should be a blank like before the Link tag.

> Link:https://github.com/KSPP/linux/issues/90

But not between it and other tags.

Also, there should be a space after "Link:"

Link: https://github.com/KSPP/linux/issues/90
> Signed-off-by: Baris Can Goral <goralbaris@...il.com>
> ---
>  net/rds/connection.c | 4 ++--
>  net/rds/stats.c      | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c749c5525b40..fb2f14a1279a 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -749,7 +749,7 @@ static int rds_conn_info_visitor(struct rds_conn_path *cp, void *buffer)
>  	cinfo->laddr = conn->c_laddr.s6_addr32[3];
>  	cinfo->faddr = conn->c_faddr.s6_addr32[3];
>  	cinfo->tos = conn->c_tos;
> -	strncpy(cinfo->transport, conn->c_trans->t_name,
> +	strscpy(cinfo->transport, conn->c_trans->t_name,
>  		sizeof(cinfo->transport));

I agree that strscpy() is appropriate as we want null termination
but not padding.

Because the destination, cinfo->transport, is an array I believe
we can omit passing the size argument to strscpy, like this:

	strscpy(cinfo->transport, conn->c_trans->t_name);

Link: https://docs.kernel.org/core-api/kernel-api.html#c.strscpy

>  	cinfo->flags = 0;
>  
> @@ -775,7 +775,7 @@ static int rds6_conn_info_visitor(struct rds_conn_path *cp, void *buffer)
>  	cinfo6->next_rx_seq = cp->cp_next_rx_seq;
>  	cinfo6->laddr = conn->c_laddr;
>  	cinfo6->faddr = conn->c_faddr;
> -	strncpy(cinfo6->transport, conn->c_trans->t_name,
> +	strscpy(cinfo6->transport, conn->c_trans->t_name,
>  		sizeof(cinfo6->transport));
>  	cinfo6->flags = 0;

Ditto.

>  
> diff --git a/net/rds/stats.c b/net/rds/stats.c
> index 9e87da43c004..63c34dbdf97f 100644
> --- a/net/rds/stats.c
> +++ b/net/rds/stats.c
> @@ -89,7 +89,7 @@ void rds_stats_info_copy(struct rds_info_iterator *iter,
>  
>  	for (i = 0; i < nr; i++) {
>  		BUG_ON(strlen(names[i]) >= sizeof(ctr.name));
> -		strncpy(ctr.name, names[i], sizeof(ctr.name) - 1);
> +		strscpy(ctr.name, names[i], sizeof(ctr.name) - 1);
>  		ctr.name[sizeof(ctr.name) - 1] = '\0';
>  		ctr.value = values[i];

This issue appears to have been addressed by
commit c451715d78e3 ("net/rds: Replace deprecated strncpy() with strscpy_pad()")

As a Networking patch please make sure it is based on the net-next tree and
targeted at that tree like this:

  Subject: [PATCH net-next v2] ...

Or the net tree if it is a big fix, which this patch isn't.

Please consider posting a v2 patch to address the above.
And please consider CCing linux-rdma@...r.kernel.org on v2.

-- 
pw-bot: changes-requested