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

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

[Simon Horman]

+ linux-rdma@vger.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@gmail.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@vger.kernel.org on v2.

-- 
pw-bot: changes-requested

[PATCH net-next v2] Replace strncpy with strscpy

[Baris Can Goral]

The strncpy() function is actively dangerous to use since it may not
NULL-terminate the destination string, resulting in potential memory.
Link: https://github.com/KSPP/linux/issues/90

Signed-off-by: Baris Can Goral <goralbaris@gmail.com>
---
 net/rds/connection.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index c749c5525b40..4bb727dd13b0 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -749,8 +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,
-		sizeof(cinfo->transport));
+	strscpy(cinfo->transport, conn->c_trans->t_name);
 	cinfo->flags = 0;
 
 	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
@@ -775,7 +774,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;
 
-- 
2.34.1

[PATCH net-next v2] Replace strncpy with strscpy

[goralbaris]

Hi,
Any news about this Patch?

Best Regards,
Baris

Re: [PATCH net-next v2] Replace strncpy with strscpy

[Simon Horman]

On Sat, May 17, 2025 at 09:12:48PM +0300, goralbaris wrote:
> Hi,
> Any news about this Patch?

Hi Baris,

I expect this patch floundered because it was not CCed to netdev.

In any case, there was a separate effort to address this problem.

- [PATCH v2] net: rds: Replace strncpy with strscpy in connection setup
  by Shankari Anand
  https://lore.kernel.org/netdev/20250426192113.47012-1-shankari.ak0208@gmail.com/

I would suggest that a successful patch should:

* Use the two-argument variant of strscpy_pad (see thread above)
* Include commentary in the commit message regarding
  - Why strctpy* is preferred over strncpy (you already did that :).
  - Why strscpy_pad is appropriate instead of strscpy
* CC all relevant parties, including netdev, the Netdev maintainers,
  Shankari Anand and Allison Henderson

[PATCH v3 net-next: rds] replace strncpy with strscpy_pad

[goralbaris]

The strncpy() function is actively dangerous to use since it may not
NULL-terminate the destination string, resulting in potential memory.
Link: https://github.com/KSPP/linux/issues/90

In addition, strscpy_pad is more appropriate because it also zero-fills 
any remaining space in the destination if the source is shorter than 
the provided buffer size.

Signed-off-by: goralbaris <goralbaris@gmail.com>
---
 net/rds/connection.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index c749c5525b40..d62f486ab29f 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -749,8 +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,
-		sizeof(cinfo->transport));
+	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
 	cinfo->flags = 0;
 
 	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
@@ -775,8 +774,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,
-		sizeof(cinfo6->transport));
+	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
 	cinfo6->flags = 0;
 
 	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
-- 
2.34.1

Re: [PATCH v3 net-next: rds] replace strncpy with strscpy_pad

[Michal Swiatkowski]

On Sun, May 18, 2025 at 10:53:29PM +0300, goralbaris wrote:
> The strncpy() function is actively dangerous to use since it may not
> NULL-terminate the destination string, resulting in potential memory.
> Link: https://github.com/KSPP/linux/issues/90
> 
> In addition, strscpy_pad is more appropriate because it also zero-fills 
> any remaining space in the destination if the source is shorter than 
> the provided buffer size.
> 
> Signed-off-by: goralbaris <goralbaris@gmail.com>

There should be your full name, not nick.

Feel free to add my RB tag
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

> ---
>  net/rds/connection.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c749c5525b40..d62f486ab29f 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -749,8 +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,
> -		sizeof(cinfo->transport));
> +	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
>  	cinfo->flags = 0;
>  
>  	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
> @@ -775,8 +774,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,
> -		sizeof(cinfo6->transport));
> +	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
>  	cinfo6->flags = 0;
>  
>  	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
> -- 
> 2.34.1

[PATCH v4 net-next: rds] replace strncpy with strscpy_pad

[Baris Can Goral]

The strncpy() function is actively dangerous to use since it may not
NULL-terminate the destination string, resulting in potential memory.
Link: https://github.com/KSPP/linux/issues/90

In addition, strscpy_pad is more appropriate because it also zero-fills
any remaining space in the destination if the source is shorter than
the provided buffer size.

Signed-off-by: Baris Can Goral <goralbaris@gmail.com>
---
 net/rds/connection.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index c749c5525b40..d62f486ab29f 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -749,8 +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,
-		sizeof(cinfo->transport));
+	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
 	cinfo->flags = 0;
 
 	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
@@ -775,8 +774,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,
-		sizeof(cinfo6->transport));
+	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
 	cinfo6->flags = 0;
 
 	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
-- 
2.34.1

Re: [PATCH v4 net-next: rds] replace strncpy with strscpy_pad

[Allison Henderson]

On Mon, 2025-05-19 at 15:51 +0300, Baris Can Goral wrote:
> The strncpy() function is actively dangerous to use since it may not
> NULL-terminate the destination string, resulting in potential memory.
It looks like we lost the last part of this phrase? I think you meant to quote the link: "...potential memory content
exposures, unbounded reads, or crashes."

Other than that I think it looks ok.  Thanks!
Allison

> Link: https://urldefense.com/v3/__https://github.com/KSPP/linux/issues/90__;!!ACWV5N9M2RV99hQ!Ja5aVj9u5vDpeBsiMWIGFvGhVzCbcj-gUOS9qIbQ_QDVAy_GU9E4yl_yCjzYJ61uEfuo368zv8bY5vsmB9yxR-7h$ 
> 
> In addition, strscpy_pad is more appropriate because it also zero-fills
> any remaining space in the destination if the source is shorter than
> the provided buffer size.
> 
> Signed-off-by: Baris Can Goral <goralbaris@gmail.com>
> ---
>  net/rds/connection.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c749c5525b40..d62f486ab29f 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -749,8 +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,
> -		sizeof(cinfo->transport));
> +	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
>  	cinfo->flags = 0;
>  
>  	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
> @@ -775,8 +774,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,
> -		sizeof(cinfo6->transport));
> +	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
>  	cinfo6->flags = 0;
>  
>  	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),

[PATCH v5 net-next: rds] replace strncpy with strscpy_pad

[Baris Can Goral]

The strncpy() function is actively dangerous to use since it may not
NULL-terminate the destination string, resulting in potential memory
content exposures, unbounded reads, or crashes.
Link: https://github.com/KSPP/linux/issues/90

In addition, strscpy_pad is more appropriate because it also zero-fills
any remaining space in the destination if the source is shorter than
the provided buffer size.

Signed-off-by: Baris Can Goral <goralbaris@gmail.com>
---
 net/rds/connection.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index c749c5525b40..d62f486ab29f 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -749,8 +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,
-		sizeof(cinfo->transport));
+	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
 	cinfo->flags = 0;
 
 	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
@@ -775,8 +774,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,
-		sizeof(cinfo6->transport));
+	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
 	cinfo6->flags = 0;
 
 	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
-- 
2.34.1

Re: [PATCH v5 net-next: rds] replace strncpy with strscpy_pad

[Zhu Yanjun]

在 2025/5/20 18:23, Baris Can Goral 写道:
> The strncpy() function is actively dangerous to use since it may not
> NULL-terminate the destination string, resulting in potential memory
> content exposures, unbounded reads, or crashes.
> Link: https://github.com/KSPP/linux/issues/90
> 
> In addition, strscpy_pad is more appropriate because it also zero-fills
> any remaining space in the destination if the source is shorter than
> the provided buffer size.

Please don't reply to an old thread when starting a new version.

It is better to start a new thread with the new version.

Zhu Yanjun
> 
> Signed-off-by: Baris Can Goral <goralbaris@gmail.com>
> ---
>   net/rds/connection.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c749c5525b40..d62f486ab29f 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -749,8 +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,
> -		sizeof(cinfo->transport));
> +	strscpy_pad(cinfo->transport, conn->c_trans->t_name);
>   	cinfo->flags = 0;
>   
>   	rds_conn_info_set(cinfo->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),
> @@ -775,8 +774,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,
> -		sizeof(cinfo6->transport));
> +	strscpy_pad(cinfo6->transport, conn->c_trans->t_name);
>   	cinfo6->flags = 0;
>   
>   	rds_conn_info_set(cinfo6->flags, test_bit(RDS_IN_XMIT, &cp->cp_flags),