| Message ID | 20240602023754.25443-5-laoar.shao@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kernel: Avoid memcpy of task comm | expand |
On Sat, Jun 1, 2024 at 10:38 PM Yafang Shao <laoar.shao@gmail.com> wrote: > > Quoted from Linus [0]: > > selinux never wanted a lock, and never wanted any kind of *consistent* > result, it just wanted a *stable* result. > > Using __get_task_comm() to read the task comm ensures that the name is > always NUL-terminated, regardless of the source string. This approach also > facilitates future extensions to the task comm. > > Signed-off-by: Yafang Shao <laoar.shao@gmail.com> > LINK: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com/ [0] > Cc: Paul Moore <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> > Cc: Stephen Smalley <stephen.smalley.work@gmail.com> > Cc: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/lsm_audit.c | 4 ++-- > security/selinux/selinuxfs.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) Similar to the audit change, as long as you sort out the __get_task_comm() issues such that it can operate without task_lock() this should be fine. Acked-by: Paul Moore <paul@paul-moore.com> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c > index 849e832719e2..a922e4339dd5 100644 > --- a/security/lsm_audit.c > +++ b/security/lsm_audit.c > @@ -207,7 +207,7 @@ static void dump_common_audit_data(struct audit_buffer *ab, > BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2); > > audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current)); > - audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm))); > + audit_log_untrustedstring(ab, __get_task_comm(comm, sizeof(comm), current)); > > switch (a->type) { > case LSM_AUDIT_DATA_NONE: > @@ -302,7 +302,7 @@ static void dump_common_audit_data(struct audit_buffer *ab, > char comm[sizeof(tsk->comm)]; > audit_log_format(ab, " opid=%d ocomm=", pid); > audit_log_untrustedstring(ab, > - memcpy(comm, tsk->comm, sizeof(comm))); > + __get_task_comm(comm, sizeof(comm), tsk)); > } > } > break; > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c > index e172f182b65c..a8a2ec742576 100644 > --- a/security/selinux/selinuxfs.c > +++ b/security/selinux/selinuxfs.c > @@ -708,7 +708,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, > if (new_value) { > char comm[sizeof(current->comm)]; > > - memcpy(comm, current->comm, sizeof(comm)); > + __get_task_comm(comm, sizeof(comm), current); > pr_err("SELinux: %s (%d) set checkreqprot to 1. This is no longer supported.\n", > comm, current->pid); > } > -- > 2.39.1
diff --git a/security/lsm_audit.c b/security/lsm_audit.c index 849e832719e2..a922e4339dd5 100644 --- a/security/lsm_audit.c +++ b/security/lsm_audit.c @@ -207,7 +207,7 @@ static void dump_common_audit_data(struct audit_buffer *ab, BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2); audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current)); - audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm))); + audit_log_untrustedstring(ab, __get_task_comm(comm, sizeof(comm), current)); switch (a->type) { case LSM_AUDIT_DATA_NONE: @@ -302,7 +302,7 @@ static void dump_common_audit_data(struct audit_buffer *ab, char comm[sizeof(tsk->comm)]; audit_log_format(ab, " opid=%d ocomm=", pid); audit_log_untrustedstring(ab, - memcpy(comm, tsk->comm, sizeof(comm))); + __get_task_comm(comm, sizeof(comm), tsk)); } } break; diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index e172f182b65c..a8a2ec742576 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -708,7 +708,7 @@ static ssize_t sel_write_checkreqprot(struct file *file, const char __user *buf, if (new_value) { char comm[sizeof(current->comm)]; - memcpy(comm, current->comm, sizeof(comm)); + __get_task_comm(comm, sizeof(comm), current); pr_err("SELinux: %s (%d) set checkreqprot to 1. This is no longer supported.\n", comm, current->pid); }
Quoted from Linus [0]: selinux never wanted a lock, and never wanted any kind of *consistent* result, it just wanted a *stable* result. Using __get_task_comm() to read the task comm ensures that the name is always NUL-terminated, regardless of the source string. This approach also facilitates future extensions to the task comm. Signed-off-by: Yafang Shao <laoar.shao@gmail.com> LINK: https://lore.kernel.org/all/CAHk-=wivfrF0_zvf+oj6==Sh=-npJooP8chLPEfaFV0oNYTTBA@mail.gmail.com/ [0] Cc: Paul Moore <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: Stephen Smalley <stephen.smalley.work@gmail.com> Cc: Ondrej Mosnacek <omosnace@redhat.com> --- security/lsm_audit.c | 4 ++-- security/selinux/selinuxfs.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)