| Message ID | 20240321-strncpy-fs-binfmt_elf_fdpic-c-v1-1-fdde26c8989e@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | binfmt: replace deprecated strncpy with strscpy_pad | expand |
Justin Stitt <justinstitt@google.com> writes: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > In every other location psinfo->pr_fname is used, it's with strscpy_pad. > It's clear that this field needs to be NUL-terminated and potentially > NUL-padded as well. > binfmt_elf.c +1545: > | char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) > | { > | task_lock(tsk); > | /* Always NUL terminated and zero-padded */ > | strscpy_pad(buf, tsk->comm, buf_size); > | task_unlock(tsk); > | return buf; > | } > > Note that this patch relies on the _new_ 2-argument versions of > strscpy() and strscpy_pad() introduced in Commit e6584c3964f2f ("string: > Allow 2-argument strscpy()"). I am perplexed. Why not use get_task_comm fill_psinfo like binfmt_elf does? It seems very silly to copy half the function without locking and then not copy it's locking as well. Given that the more highly tested binfmt_elf uses get_task_comm I can't imagine a reason why binfmt_elf_fdpic can't use it as well. Eric > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> > --- > Note: build-tested only. > > Found with: $ rg "strncpy\(" > --- > fs/binfmt_elf_fdpic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c > index 1920ed69279b..0365f14f18fc 100644 > --- a/fs/binfmt_elf_fdpic.c > +++ b/fs/binfmt_elf_fdpic.c > @@ -1359,7 +1359,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p, > SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid)); > SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid)); > rcu_read_unlock(); > - strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname)); > + strscpy_pad(psinfo->pr_fname, p->comm); > > return 0; > } > > --- > base-commit: a4145ce1e7bc247fd6f2846e8699473448717b37 > change-id: 20240320-strncpy-fs-binfmt_elf_fdpic-c-828286d76310 > > Best regards, > -- > Justin Stitt <justinstitt@google.com>
Hi, On Thu, Mar 21, 2024 at 9:23 AM Eric W. Biederman <ebiederm@xmission.com> wrote: > > I am perplexed. Why not use get_task_comm fill_psinfo like binfmt_elf > does? > > It seems very silly to copy half the function without locking and then > not copy it's locking as well. > > Given that the more highly tested binfmt_elf uses get_task_comm I can't > imagine a reason why binfmt_elf_fdpic can't use it as well. I am not sure why the original opted for strncpy over get_task_comm but I made the replacement without being aware of the literally identical code present in binfmt_elf.c I'll send a v2. > > Eric Thanks Justin
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 1920ed69279b..0365f14f18fc 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -1359,7 +1359,7 @@ static int fill_psinfo(struct elf_prpsinfo *psinfo, struct task_struct *p, SET_UID(psinfo->pr_uid, from_kuid_munged(cred->user_ns, cred->uid)); SET_GID(psinfo->pr_gid, from_kgid_munged(cred->user_ns, cred->gid)); rcu_read_unlock(); - strncpy(psinfo->pr_fname, p->comm, sizeof(psinfo->pr_fname)); + strscpy_pad(psinfo->pr_fname, p->comm); return 0; }
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. In every other location psinfo->pr_fname is used, it's with strscpy_pad. It's clear that this field needs to be NUL-terminated and potentially NUL-padded as well. binfmt_elf.c +1545: | char *__get_task_comm(char *buf, size_t buf_size, struct task_struct *tsk) | { | task_lock(tsk); | /* Always NUL terminated and zero-padded */ | strscpy_pad(buf, tsk->comm, buf_size); | task_unlock(tsk); | return buf; | } Note that this patch relies on the _new_ 2-argument versions of strscpy() and strscpy_pad() introduced in Commit e6584c3964f2f ("string: Allow 2-argument strscpy()"). Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Note: build-tested only. Found with: $ rg "strncpy\(" --- fs/binfmt_elf_fdpic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: a4145ce1e7bc247fd6f2846e8699473448717b37 change-id: 20240320-strncpy-fs-binfmt_elf_fdpic-c-828286d76310 Best regards, -- Justin Stitt <justinstitt@google.com>