| Message ID | 20230809-net-netfilter-v2-1-5847d707ec0a@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netfilter: refactor deprecated strncpy | expand |
Justin Stitt <justinstitt@google.com> wrote:
> Use `strscpy_pad` instead of `strncpy`.
I don't think that any of these need zero-padding.
On Wed, Aug 9, 2023 at 1:19 PM Florian Westphal <fw@strlen.de> wrote: > > Justin Stitt <justinstitt@google.com> wrote: > > Use `strscpy_pad` instead of `strncpy`. > > I don't think that any of these need zero-padding. It's a more consistent change with the rest of the series and I don't believe it has much different behavior to `strncpy` (other than NUL-termination) as that will continue to pad to `n` as well. Do you think the `_pad` for 1/7, 6/7 and 7/7 should be changed back to `strscpy` in a v3? I really am shooting in the dark as it is quite hard to tell whether or not a buffer is expected to be NUL-padded or not.
On Wednesday 2023-08-09 23:40, Justin Stitt wrote: >On Wed, Aug 9, 2023 at 1:19 PM Florian Westphal <fw@strlen.de> wrote: >> >> Justin Stitt <justinstitt@google.com> wrote: >> > Use `strscpy_pad` instead of `strncpy`. >> >> I don't think that any of these need zero-padding. >It's a more consistent change with the rest of the series and I don't >believe it has much different behavior to `strncpy` (other than >NUL-termination) as that will continue to pad to `n` as well. > >Do you think the `_pad` for 1/7, 6/7 and 7/7 should be changed back to >`strscpy` in a v3? I really am shooting in the dark as it is quite >hard to tell whether or not a buffer is expected to be NUL-padded or >not. I don't recall either NF userspace or kernelspace code doing memcmp with name-like fields, so padding should not be strictly needed.
Justin Stitt <justinstitt@google.com> wrote: > On Wed, Aug 9, 2023 at 1:19 PM Florian Westphal <fw@strlen.de> wrote: > > > > Justin Stitt <justinstitt@google.com> wrote: > > > Use `strscpy_pad` instead of `strncpy`. > > > > I don't think that any of these need zero-padding. > It's a more consistent change with the rest of the series and I don't > believe it has much different behavior to `strncpy` (other than > NUL-termination) as that will continue to pad to `n` as well. > > Do you think the `_pad` for 1/7, 6/7 and 7/7 should be changed back to > `strscpy` in a v3? I really am shooting in the dark as it is quite > hard to tell whether or not a buffer is expected to be NUL-padded or > not. No, you can keep it as-is. Which tree are you targetting with this?
On Wed, Aug 9, 2023 at 2:58 PM Florian Westphal <fw@strlen.de> wrote: > > Justin Stitt <justinstitt@google.com> wrote: > > On Wed, Aug 9, 2023 at 1:19 PM Florian Westphal <fw@strlen.de> wrote: > > > > > > Justin Stitt <justinstitt@google.com> wrote: > > > > Use `strscpy_pad` instead of `strncpy`. > > > > > > I don't think that any of these need zero-padding. > > It's a more consistent change with the rest of the series and I don't > > believe it has much different behavior to `strncpy` (other than > > NUL-termination) as that will continue to pad to `n` as well. > > > > Do you think the `_pad` for 1/7, 6/7 and 7/7 should be changed back to > > `strscpy` in a v3? I really am shooting in the dark as it is quite > > hard to tell whether or not a buffer is expected to be NUL-padded or > > not. > > No, you can keep it as-is. Which tree are you targetting with this? Not sure, I let ./getmaintainer auto-add the mailing lists. Perhaps netdev or netfilter next trees?
On Wed, Aug 09, 2023 at 11:54:48PM +0200, Jan Engelhardt wrote: > > On Wednesday 2023-08-09 23:40, Justin Stitt wrote: > >On Wed, Aug 9, 2023 at 1:19 PM Florian Westphal <fw@strlen.de> wrote: > >> > >> Justin Stitt <justinstitt@google.com> wrote: > >> > Use `strscpy_pad` instead of `strncpy`. > >> > >> I don't think that any of these need zero-padding. > >It's a more consistent change with the rest of the series and I don't > >believe it has much different behavior to `strncpy` (other than > >NUL-termination) as that will continue to pad to `n` as well. > > > >Do you think the `_pad` for 1/7, 6/7 and 7/7 should be changed back to > >`strscpy` in a v3? I really am shooting in the dark as it is quite > >hard to tell whether or not a buffer is expected to be NUL-padded or > >not. > > I don't recall either NF userspace or kernelspace code doing memcmp > with name-like fields, so padding should not be strictly needed. My only concern with padding is just to make sure any buffers copied to userspace have been zeroed. I would need to take a close look at how buffers are passed around here to know for sure...
diff --git a/net/netfilter/ipset/ip_set_core.c b/net/netfilter/ipset/ip_set_core.c index 0b68e2e2824e..e564b5174261 100644 --- a/net/netfilter/ipset/ip_set_core.c +++ b/net/netfilter/ipset/ip_set_core.c @@ -872,7 +872,7 @@ ip_set_name_byindex(struct net *net, ip_set_id_t index, char *name) BUG_ON(!set); read_lock_bh(&ip_set_ref_lock); - strncpy(name, set->name, IPSET_MAXNAMELEN); + strscpy_pad(name, set->name, IPSET_MAXNAMELEN); read_unlock_bh(&ip_set_ref_lock); } EXPORT_SYMBOL_GPL(ip_set_name_byindex); @@ -1326,7 +1326,7 @@ static int ip_set_rename(struct sk_buff *skb, const struct nfnl_info *info, goto out; } } - strncpy(set->name, name2, IPSET_MAXNAMELEN); + strscpy_pad(set->name, name2, IPSET_MAXNAMELEN); out: write_unlock_bh(&ip_set_ref_lock); @@ -1380,9 +1380,9 @@ static int ip_set_swap(struct sk_buff *skb, const struct nfnl_info *info, return -EBUSY; } - strncpy(from_name, from->name, IPSET_MAXNAMELEN); - strncpy(from->name, to->name, IPSET_MAXNAMELEN); - strncpy(to->name, from_name, IPSET_MAXNAMELEN); + strscpy_pad(from_name, from->name, IPSET_MAXNAMELEN); + strscpy_pad(from->name, to->name, IPSET_MAXNAMELEN); + strscpy_pad(to->name, from_name, IPSET_MAXNAMELEN); swap(from->ref, to->ref); ip_set(inst, from_id) = to;
Use `strscpy_pad` instead of `strncpy`. Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- net/netfilter/ipset/ip_set_core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)