| Message ID | 20240711171652.work.887-kees@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/ipv4: Replace tcp_ca_get_name_by_key()'s strncpy() with strscpy() | expand |
On Thu, Jul 11, 2024 at 10:16 AM Kees Cook <kees@kernel.org> wrote: > > Replace the deprecated[1] use of strncpy() in tcp_ca_get_name_by_key(). > The only caller passes the results to nla_put_string(), so trailing > padding is not needed. > > Since passing "buffer" decays it to a pointer, the size can't be > trivially determined by the compiler. ca->name is the same length, > 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> > --- > 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 | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c > index 28ffcfbeef14..2a303a7cba59 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(); > Ok, but what about tcp_get_default_congestion_control() ? Thanks.
On Thu, Jul 11, 2024 at 10:38:01AM -0700, Eric Dumazet wrote: > On Thu, Jul 11, 2024 at 10:16 AM Kees Cook <kees@kernel.org> wrote: > > > > Replace the deprecated[1] use of strncpy() in tcp_ca_get_name_by_key(). > > The only caller passes the results to nla_put_string(), so trailing > > padding is not needed. > > > > Since passing "buffer" decays it to a pointer, the size can't be > > trivially determined by the compiler. ca->name is the same length, > > 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> > > --- > > 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 | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c > > index 28ffcfbeef14..2a303a7cba59 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(); > > > > Ok, but what about tcp_get_default_congestion_control() ? Whoops. Yes. I'll do that at the same time. v2 coming...
diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c index 28ffcfbeef14..2a303a7cba59 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;
Replace the deprecated[1] use of strncpy() in tcp_ca_get_name_by_key(). The only caller passes the results to nla_put_string(), so trailing padding is not needed. Since passing "buffer" decays it to a pointer, the size can't be trivially determined by the compiler. ca->name is the same length, 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> --- 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 | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)