Skip to contentSkip to conversation

net/ipv4/tcp_cong: Replace strncpy() with strscpy()

PATCH Applied Kees Cook 5 participants 5 messages Jul 14, 2024 – Jul 16, 2024

Subsystems (3)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH v2 1/1 net/ipv4/tcp_cong: Replace strncpy() with strscpy()

Replace the deprecated[1] uses of strncpy() in tcp_ca_get_name_by_key() and tcp_get_default_congestion_control(). The callers use the results as standard C strings (via nla_put_string() and proc handlers respectively), so trailing padding is not needed.

Since passing the destination buffer arguments decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length in both cases, so strscpy() won't fail (when ca->name is NUL-terminated). Include the length explicitly instead of using the 2-argument strscpy().

---
v2: add tcp_get_default_congestion_control() conversion
v1: https://lore.kernel.org/lkml/[email protected]/
Cc: Eric Dumazet <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: [email protected]

---
 net/ipv4/tcp_cong.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
net/ipv4/tcp_cong.c +5 -4
index 28ffcfbeef14..874531c7c08b 100644
@@ -203,9 +203,10 @@ char *tcp_ca_get_name_by_key(u32 key, char *buffer)
 
 	rcu_read_lock();
 	ca = tcp_ca_find_key(key);
-	if (ca)
-		ret = strncpy(buffer, ca->name,
-			      TCP_CA_NAME_MAX);
+	if (ca) {
+		strscpy(buffer, ca->name, TCP_CA_NAME_MAX);
+		ret = buffer;
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -338,7 +339,7 @@ void tcp_get_default_congestion_control(struct net *net, char *name)
 
 	rcu_read_lock();
 	ca = rcu_dereference(net->ipv4.tcp_congestion_control);
-	strncpy(name, ca->name, TCP_CA_NAME_MAX);
+	strscpy(name, ca->name, TCP_CA_NAME_MAX);
 	rcu_read_unlock();
 }
 
Signature · 1 line
2.34.1
reviewer Reviewed-by on v2 1/1
#1 lore raw
Quoted 5 lines · Kees Cook

Replace the deprecated[1] uses of strncpy() in tcp_ca_get_name_by_key() and tcp_get_default_congestion_control(). The callers use the results as standard C strings (via nla_put_string() and proc handlers respectively), so trailing padding is not needed.

Quoted 8 lines

Since passing the destination buffer arguments decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length in both cases, so strscpy() won't fail (when ca->name is NUL-terminated). Include the length explicitly instead of using the 2-argument strscpy().

nit: Looking at git history, the subject prefix should probably be 'tcp'.
And it would be best to explicitly target the patch against net-next.

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

That notwithstanding, this looks good to me.

...

maintainer
#2 lore raw
Quoted 15 lines · Simon Horman
Quoted 5 lines · Kees Cook

Replace the deprecated[1] uses of strncpy() in tcp_ca_get_name_by_key() and tcp_get_default_congestion_control(). The callers use the results as standard C strings (via nla_put_string() and proc handlers respectively), so trailing padding is not needed.

Quoted 8 lines

Since passing the destination buffer arguments decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length in both cases, so strscpy() won't fail (when ca->name is NUL-terminated). Include the length explicitly instead of using the 2-argument strscpy().

Quoted 8 lines

nit: Looking at git history, the subject prefix should probably be 'tcp'.
And it would be best to explicitly target the patch against net-next.

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

That notwithstanding, this looks good to me.

@Eric: I can fix the prefix when applying the patch. Please LMK if you prefer otherwise.

Thanks,

Paolo

Reviewed-by
#3 lore raw
Quoted 20 lines · Paolo Abeni
Quoted 18 lines · Simon Horman
Quoted 5 lines · Kees Cook

Replace the deprecated[1] uses of strncpy() in tcp_ca_get_name_by_key() and tcp_get_default_congestion_control(). The callers use the results as standard C strings (via nla_put_string() and proc handlers respectively), so trailing padding is not needed.

Quoted 8 lines

Since passing the destination buffer arguments decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length in both cases, so strscpy() won't fail (when ca->name is NUL-terminated). Include the length explicitly instead of using the 2-argument strscpy().

nit: Looking at git history, the subject prefix should probably be 'tcp'.
And it would be best to explicitly target the patch against net-next.

Quoted 8 lines
Quoted 5 lines

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

That notwithstanding, this looks good to me.

@Eric: I can fix the prefix when applying the patch. Please LMK if you prefer otherwise.

Sure thing, thanks for taking care of this Paolo, Simon, and Kees.

#4 lore raw
In reply to Kees Cook
This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 5 lines · On Sat, 13 Jul 2024 21:11:15 -0700 you wrote:

Replace the deprecated[1] uses of strncpy() in tcp_ca_get_name_by_key() and tcp_get_default_congestion_control(). The callers use the results as standard C strings (via nla_put_string() and proc handlers respectively), so trailing padding is not needed.

Quoted 7 lines

Since passing the destination buffer arguments decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length in both cases, so strscpy() won't fail (when ca->name is NUL-terminated). Include the length explicitly instead of using the 2-argument strscpy().

[...]

Here is the summary with links:
- [v2] net/ipv4/tcp_cong: Replace strncpy() with strscpy()

    https://git.kernel.org/netdev/net-next/c/a3bfc095060b

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html