summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRichard Guy Briggs <rgb@redhat.com>2015-04-14 11:01:02 -0400
committerJiri Slaby <jslaby@suse.cz>2015-06-10 15:20:26 +0200
commit44fa042f532c7f8ac3421456eaae4255297092e0 (patch)
tree189543bd4725e2669636a8b50faed8a50cde4cc8
parent62db23cfb6e7a4860684be9d328fd8e22b4f193a (diff)
lsm: copy comm before calling audit_log to avoid race in string printing
commit 5deeb5cece3f9b30c8129786726b9d02c412c8ca upstream. When task->comm is passed directly to audit_log_untrustedstring() without getting a copy or using the task_lock, there is a race that could happen that would output a NULL (\0) in the middle of the output string that would effectively truncate the rest of the report text after the comm= field in the audit log message, losing fields. Using get_task_comm() to get a copy while acquiring the task_lock to prevent this and to prevent the result from being a mixture of old and new values of comm would incur potentially unacceptable overhead, considering that the value can be influenced by userspace and therefore untrusted anyways. Copy the value before passing it to audit_log_untrustedstring() ensures that a local copy is used to calculate the length *and* subsequently printed. Even if this value contains a mix of old and new values, it will only calculate and copy up to the first NULL, preventing the rest of the audit log message being truncated. Use a second local copy of comm to avoid a race between the first and second calls to audit_log_untrustedstring() with comm. Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Signed-off-by: James Morris <james.l.morris@oracle.com> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
-rw-r--r--security/lsm_audit.c15
1 files changed, 9 insertions, 6 deletions
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 8d8d97dbb389..7537013d4042 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -211,7 +211,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
static void dump_common_audit_data(struct audit_buffer *ab,
struct common_audit_data *a)
{
- struct task_struct *tsk = current;
+ char comm[sizeof(current->comm)];
/*
* To keep stack sizes in check force programers to notice if they
@@ -220,8 +220,8 @@ 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=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_format(ab, " pid=%d comm=", current->pid);
+ audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
switch (a->type) {
case LSM_AUDIT_DATA_NONE:
@@ -276,13 +276,16 @@ static void dump_common_audit_data(struct audit_buffer *ab,
audit_log_format(ab, " ino=%lu", inode->i_ino);
break;
}
- case LSM_AUDIT_DATA_TASK:
- tsk = a->u.tsk;
+ case LSM_AUDIT_DATA_TASK: {
+ struct task_struct *tsk = a->u.tsk;
if (tsk && tsk->pid) {
+ char comm[sizeof(tsk->comm)];
audit_log_format(ab, " pid=%d comm=", tsk->pid);
- audit_log_untrustedstring(ab, tsk->comm);
+ audit_log_untrustedstring(ab,
+ memcpy(comm, tsk->comm, sizeof(comm)));
}
break;
+ }
case LSM_AUDIT_DATA_NET:
if (a->u.net->sk) {
struct sock *sk = a->u.net->sk;