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

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

[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.

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().

Link: https://github.com/KSPP/linux/issues/90 [1]
Signed-off-by: Kees Cook <kees@kernel.org>
---
 v2: add tcp_get_default_congestion_control() conversion
 v1: https://lore.kernel.org/lkml/20240711171652.work.887-kees@kernel.org/
Cc: Eric Dumazet <edumazet@google.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: David Ahern <dsahern@kernel.org>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
---
 net/ipv4/tcp_cong.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
index 28ffcfbeef14..874531c7c08b 100644
--- a/net/ipv4/tcp_cong.c
+++ b/net/ipv4/tcp_cong.c
@@ -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();
 }
 
-- 
2.34.1

Re: [PATCH v2] net/ipv4/tcp_cong: Replace strncpy() with strscpy()

[Simon Horman]

On Sat, Jul 13, 2024 at 09:11:15PM -0700, Kees Cook 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.
> 
> 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().
> 
> Link: https://github.com/KSPP/linux/issues/90 [1]
> Signed-off-by: Kees Cook <kees@kernel.org>

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.

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH v2] net/ipv4/tcp_cong: Replace strncpy() with strscpy()

[Paolo Abeni]

On 7/15/24 11:41, Simon Horman wrote:
> On Sat, Jul 13, 2024 at 09:11:15PM -0700, Kees Cook 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.
>>
>> 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().
>>
>> Link: https://github.com/KSPP/linux/issues/90 [1]
>> Signed-off-by: Kees Cook <kees@kernel.org>
> 
> 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.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>

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

Thanks,

Paolo

Re: [PATCH v2] net/ipv4/tcp_cong: Replace strncpy() with strscpy()

[Eric Dumazet]

On Tue, Jul 16, 2024 at 4:32 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 7/15/24 11:41, Simon Horman wrote:
> > On Sat, Jul 13, 2024 at 09:11:15PM -0700, Kees Cook 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.
> >>
> >> 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().
> >>
> >> Link: https://github.com/KSPP/linux/issues/90 [1]
> >> Signed-off-by: Kees Cook <kees@kernel.org>
> >
> > 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.
> >
> > Reviewed-by: Simon Horman <horms@kernel.org>
>
> @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.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH v2] net/ipv4/tcp_cong: Replace strncpy() with strscpy()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

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.
> 
> 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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html