| Message ID | 20230808-net-netfilter-v1-6-efbbe4ec60af@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | netfilter: refactor deprecated strncpy | expand |
Justin Stitt <justinstitt@google.com> wrote: > Prefer `strscpy` to `strncpy` for use on NUL-terminated destination > buffers. > > This fixes a potential bug due to the fact that both `t->u.user.name` > and `name` share the same size. This replacement seems fine. > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > Here's an example of what happens when dest and src share same size: > | #define MAXLEN 5 > | char dest[MAXLEN]; > | const char *src = "hello"; > | strncpy(dest, src, MAXLEN); // -> should use strscpy() > | // dest is now not NUL-terminated This can't happen here, the source string is coming from the kernel (xt target and matchinfo struct). But, even if it would it should be fine, this function prepares the translated 64bit blob which gets passed to translate_table(), and that function has to check for '\0' presence. Normally it handles the native (non-compat) data originating from userspace, so m-->user.name can not be assumed to contain a \0.
On Tue, Aug 08, 2023 at 10:48:11PM +0000, Justin Stitt wrote: > Prefer `strscpy` to `strncpy` for use on NUL-terminated destination > buffers. > > This fixes a potential bug due to the fact that both `t->u.user.name` > and `name` share the same size. > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > --- > Here's an example of what happens when dest and src share same size: > | #define MAXLEN 5 > | char dest[MAXLEN]; > | const char *src = "hello"; > | strncpy(dest, src, MAXLEN); // -> should use strscpy() > | // dest is now not NUL-terminated > --- > net/netfilter/x_tables.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c > index 470282cf3fae..714a38ec9055 100644 > --- a/net/netfilter/x_tables.c > +++ b/net/netfilter/x_tables.c > @@ -768,7 +768,7 @@ void xt_compat_match_from_user(struct xt_entry_match *m, void **dstptr, > m->u.user.match_size = msize; > strscpy(name, match->name, sizeof(name)); > module_put(match->me); > - strncpy(m->u.user.name, name, sizeof(m->u.user.name)); > + strscpy(m->u.user.name, name, sizeof(m->u.user.name)); Two hints here are that this is dealing with user-space memory copies, so NUL-padding is needed (i.e. don't leak memory contents), and immediately above is a strscpy() already, which means there is likely some reason strncpy() is needed (i.e. padding).
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index 470282cf3fae..714a38ec9055 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -768,7 +768,7 @@ void xt_compat_match_from_user(struct xt_entry_match *m, void **dstptr, m->u.user.match_size = msize; strscpy(name, match->name, sizeof(name)); module_put(match->me); - strncpy(m->u.user.name, name, sizeof(m->u.user.name)); + strscpy(m->u.user.name, name, sizeof(m->u.user.name)); *size += off; *dstptr += msize; @@ -1148,7 +1148,7 @@ void xt_compat_target_from_user(struct xt_entry_target *t, void **dstptr, t->u.user.target_size = tsize; strscpy(name, target->name, sizeof(name)); module_put(target->me); - strncpy(t->u.user.name, name, sizeof(t->u.user.name)); + strscpy(t->u.user.name, name, sizeof(t->u.user.name)); *size += off; *dstptr += tsize; @@ -2014,4 +2014,3 @@ static void __exit xt_fini(void) module_init(xt_init); module_exit(xt_fini); -
Prefer `strscpy` to `strncpy` for use on NUL-terminated destination buffers. This fixes a potential bug due to the fact that both `t->u.user.name` and `name` share the same size. Signed-off-by: Justin Stitt <justinstitt@google.com> --- Here's an example of what happens when dest and src share same size: | #define MAXLEN 5 | char dest[MAXLEN]; | const char *src = "hello"; | strncpy(dest, src, MAXLEN); // -> should use strscpy() | // dest is now not NUL-terminated --- net/netfilter/x_tables.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)