| Message ID | 20230613003437.3538694-1-azeemshaikh38@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: ipset: Replace strlcpy with strscpy | expand |
On Tue, Jun 13, 2023 at 12:34:37AM +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > Direct replacement is safe here since return value from all > callers of STRLCPY macro were ignored. Yeah, the macro name is probably not super helpful here. It seems like it should have originally be named IFNAME_CPY or something. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> But, regardless: Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, Jun 13, 2023 at 12:34:37AM +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > Direct replacement is safe here since return value from all > callers of STRLCPY macro were ignored. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Tue, 13 Jun 2023, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > Direct replacement is safe here since return value from all > callers of STRLCPY macro were ignored. > > [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy > [2] https://github.com/KSPP/linux/issues/89 > > Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> Acked-by: Jozsef Kadlecsik <kadlec@netfilter.org> Best regards, Jozsef > --- > net/netfilter/ipset/ip_set_hash_netiface.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c > index 031073286236..95aeb31c60e0 100644 > --- a/net/netfilter/ipset/ip_set_hash_netiface.c > +++ b/net/netfilter/ipset/ip_set_hash_netiface.c > @@ -40,7 +40,7 @@ MODULE_ALIAS("ip_set_hash:net,iface"); > #define IP_SET_HASH_WITH_MULTI > #define IP_SET_HASH_WITH_NET0 > > -#define STRLCPY(a, b) strlcpy(a, b, IFNAMSIZ) > +#define STRSCPY(a, b) strscpy(a, b, IFNAMSIZ) > > /* IPv4 variant */ > > @@ -182,11 +182,11 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb, > > if (!eiface) > return -EINVAL; > - STRLCPY(e.iface, eiface); > + STRSCPY(e.iface, eiface); > e.physdev = 1; > #endif > } else { > - STRLCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); > + STRSCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); > } > > if (strlen(e.iface) == 0) > @@ -400,11 +400,11 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb, > > if (!eiface) > return -EINVAL; > - STRLCPY(e.iface, eiface); > + STRSCPY(e.iface, eiface); > e.physdev = 1; > #endif > } else { > - STRLCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); > + STRSCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); > } > > if (strlen(e.iface) == 0) > -- > 2.41.0.162.gfafddb0af9-goog > > > - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.hu PGP key : https://wigner.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics H-1525 Budapest 114, POB. 49, Hungary
On Tue, 13 Jun 2023 00:34:37 +0000, Azeem Shaikh wrote: > strlcpy() reads the entire source buffer first. > This read may exceed the destination size limit. > This is both inefficient and can lead to linear read > overflows if a source string is not NUL-terminated [1]. > In an effort to remove strlcpy() completely [2], replace > strlcpy() here with strscpy(). > > [...] Since this got Acked and it's a trivial change, I'll take this via the hardening tree. Thanks! Applied to for-next/hardening, thanks! [1/1] netfilter: ipset: Replace strlcpy with strscpy https://git.kernel.org/kees/c/0b2fa86361f4
diff --git a/net/netfilter/ipset/ip_set_hash_netiface.c b/net/netfilter/ipset/ip_set_hash_netiface.c index 031073286236..95aeb31c60e0 100644 --- a/net/netfilter/ipset/ip_set_hash_netiface.c +++ b/net/netfilter/ipset/ip_set_hash_netiface.c @@ -40,7 +40,7 @@ MODULE_ALIAS("ip_set_hash:net,iface"); #define IP_SET_HASH_WITH_MULTI #define IP_SET_HASH_WITH_NET0 -#define STRLCPY(a, b) strlcpy(a, b, IFNAMSIZ) +#define STRSCPY(a, b) strscpy(a, b, IFNAMSIZ) /* IPv4 variant */ @@ -182,11 +182,11 @@ hash_netiface4_kadt(struct ip_set *set, const struct sk_buff *skb, if (!eiface) return -EINVAL; - STRLCPY(e.iface, eiface); + STRSCPY(e.iface, eiface); e.physdev = 1; #endif } else { - STRLCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); + STRSCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); } if (strlen(e.iface) == 0) @@ -400,11 +400,11 @@ hash_netiface6_kadt(struct ip_set *set, const struct sk_buff *skb, if (!eiface) return -EINVAL; - STRLCPY(e.iface, eiface); + STRSCPY(e.iface, eiface); e.physdev = 1; #endif } else { - STRLCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); + STRSCPY(e.iface, SRCDIR ? IFACE(in) : IFACE(out)); } if (strlen(e.iface) == 0)
strlcpy() reads the entire source buffer first. This read may exceed the destination size limit. This is both inefficient and can lead to linear read overflows if a source string is not NUL-terminated [1]. In an effort to remove strlcpy() completely [2], replace strlcpy() here with strscpy(). Direct replacement is safe here since return value from all callers of STRLCPY macro were ignored. [1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strlcpy [2] https://github.com/KSPP/linux/issues/89 Signed-off-by: Azeem Shaikh <azeemshaikh38@gmail.com> --- net/netfilter/ipset/ip_set_hash_netiface.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)