| Message ID | 20181126153949.2142-1-cai@gmx.us |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [v2] netfilter: ipset: replace a strncpy() with strscpy() | expand |
Hi, On Mon, 26 Nov 2018, Qian Cai wrote: > To make overflows as obvious as possible and to prevent code from blithely > proceeding with a truncated string. This also has a side-effect to fix a > compilation warning when using GCC 8.2.1. > > net/netfilter/ipset/ip_set_core.c: In function 'ip_set_sockfn_get': > net/netfilter/ipset/ip_set_core.c:2027:3: warning: 'strncpy' writing 32 > bytes into a region of size 2 overflows the destination > [-Wstringop-overflow=] > > Signed-off-by: Qian Cai <cai@gmx.us> > --- > > Changes since v1: > * Checked the return value. > > net/netfilter/ipset/ip_set_core.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c > index 1577f2f76060..c6f82556f7f2 100644 > --- a/net/netfilter/ipset/ip_set_core.c > +++ b/net/netfilter/ipset/ip_set_core.c > @@ -2024,8 +2024,11 @@ ip_set_sockfn_get(struct sock *sk, int optval, void __user *user, int *len) > } > nfnl_lock(NFNL_SUBSYS_IPSET); > set = ip_set(inst, req_get->set.index); > - strncpy(req_get->set.name, set ? set->name : "", > - IPSET_MAXNAMELEN); > + if (strscpy(req_get->set.name, set ? set->name : "", > + IPSET_MAXNAMELEN) == -E2BIG) { > + ret = -E2BIG; > + goto done; > + } > nfnl_unlock(NFNL_SUBSYS_IPSET); > goto copy; > } This second version is not OK: the netlink lock is not released in the error path. Please use an explicit ret = strscpy() assignment first, then check the error condition and in the error path call nfnl_unlock(NFNL_SUBSYS_IPSET) and goto to the final error handling. Thanks! Best regards, Jozsef - E-mail : kadlec@blackhole.kfki.hu, kadlecsik.jozsef@wigner.mta.hu PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences H-1525 Budapest 114, POB. 49, Hungary
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index 1577f2f76060..c6f82556f7f2 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -2024,8 +2024,11 @@ ip_set_sockfn_get(struct sock *sk, int optval, void __user *user, int *len) } nfnl_lock(NFNL_SUBSYS_IPSET); set = ip_set(inst, req_get->set.index); - strncpy(req_get->set.name, set ? set->name : "", - IPSET_MAXNAMELEN); + if (strscpy(req_get->set.name, set ? set->name : "", + IPSET_MAXNAMELEN) == -E2BIG) { + ret = -E2BIG; + goto done; + } nfnl_unlock(NFNL_SUBSYS_IPSET); goto copy; }
To make overflows as obvious as possible and to prevent code from blithely proceeding with a truncated string. This also has a side-effect to fix a compilation warning when using GCC 8.2.1. net/netfilter/ipset/ip_set_core.c: In function 'ip_set_sockfn_get': net/netfilter/ipset/ip_set_core.c:2027:3: warning: 'strncpy' writing 32 bytes into a region of size 2 overflows the destination [-Wstringop-overflow=] Signed-off-by: Qian Cai <cai@gmx.us> --- Changes since v1: * Checked the return value. net/netfilter/ipset/ip_set_core.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)