audit_add_watch stores locally krule->watch without taking a reference
on watch. Then, it calls audit_add_to_parent, and uses the watch stored
locally.
Unfortunately, it is possible that audit_add_to_parent updates
krule->watch.
When it happens, it also drops a reference of watch which
could free the watch.
How to reproduce (with KASAN enabled):
auditctl -w /etc/passwd -F success=0 -k test_passwd
auditctl -w /etc/passwd -F success=1 -k test_passwd2
The second call to auditctl triggers the use-after-free, because
audit_to_parent updates krule->watch to use a previous existing watch
and drops the reference to the newly created watch.
To fix the issue, we grab a reference of watch and we release it at the
end of the function.
Signed-off-by: Ronny Chevalier <ronny.chevalier(a)hp.com>
---
v3:
- Move audit_get_watch before audit_get_nd since it is using it
and call audit_put_watch if it fails.
v2:
- Move audit_get_watch before audit_find_parent. In the case of
audit_get_nd failing.
---
kernel/audit_watch.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 6f249bdf2d84..787c7afdf829 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -420,6 +420,13 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
struct path parent_path;
int h, ret = 0;
+ /*
+ * When we will be calling audit_add_to_parent, krule->watch might have
+ * been updated and watch might have been freed.
+ * So we need to keep a reference of watch.
+ */
+ audit_get_watch(watch);
+
mutex_unlock(&audit_filter_mutex);
/* Avoid calling path_lookup under audit_filter_mutex. */
@@ -428,8 +435,10 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
/* caller expects mutex locked */
mutex_lock(&audit_filter_mutex);
- if (ret)
+ if (ret) {
+ audit_put_watch(watch);
return ret;
+ }
/* either find an old parent or attach a new one */
parent = audit_find_parent(d_backing_inode(parent_path.dentry));
@@ -447,6 +456,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
*list = &audit_inode_hash[h];
error:
path_put(&parent_path);
+ audit_put_watch(watch);
return ret;
}
--
2.18.0
Commit c72051d5778a ("audit: use ktime_get_coarse_ts64() for time
access") converted audit's use of current_kernel_time64() to the
new ktime_get_coarse_ts64() function. Unfortunately this resulted
in incorrect timestamps, e.g. events stamped with the year 1969
despite it being 2018. This patch corrects this by using
ktime_get_coarse_real_ts64() just like the current_kernel_time64()
wrapper.
Fixes: c72051d5778a ("audit: use ktime_get_coarse_ts64() for time access")
Signed-off-by: Paul Moore <paul(a)paul-moore.com>
---
kernel/audit.c | 2 +-
kernel/auditsc.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index e17bc697d11c..2a8058764aa6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1721,7 +1721,7 @@ static inline void audit_get_stamp(struct audit_context *ctx,
struct timespec64 *t, unsigned int *serial)
{
if (!ctx || !auditsc_get_stamp(ctx, t, serial)) {
- ktime_get_coarse_ts64(t);
+ ktime_get_coarse_real_ts64(t);
*serial = audit_serial();
}
}
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f6a0cb32d76e..fb207466e99b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1543,7 +1543,7 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2,
context->in_syscall = 1;
context->current_state = state;
context->ppid = 0;
- ktime_get_coarse_ts64(&context->ctime);
+ ktime_get_coarse_real_ts64(&context->ctime);
}
/**
Make some tree and watch rule logging cleanups before applying
normalizations and record connections for ghak 59.
See: https://github.com/linux-audit/audit-kernel/issues/50
Richard Guy Briggs (2):
audit: tree: check audit_enabled
audit: watch: simplify audit_enabled check
kernel/audit_tree.c | 2 ++
kernel/audit_watch.c | 29 +++++++++++++++--------------
2 files changed, 17 insertions(+), 14 deletions(-)
--
1.8.3.1
Hello,
this series addresses the problems I have identified when trying to understand
how exactly is kernel/audit_tree.c using generic fsnotify framework. I hope
I have understood all the interactions right but careful review is certainly
welcome (CCing Al as he was the one implementing this code originally).
The patches have been tested by a stress test I have written which mounts &
unmounts filesystems in the directory tree while adding and removing audit
rules for this tree in parallel and accessing the tree to generate events.
Still some real-world testing would be welcome.
Honza
audit_add_watch stores locally krule->watch without taking a reference
on watch. Then, it calls audit_add_to_parent, and uses the watch stored
locally.
Unfortunately, it is possible that audit_add_to_parent updates
krule->watch.
When it happens, it also drops a reference of watch which
could free the watch.
How to reproduce (with KASAN enabled):
auditctl -w /etc/passwd -F success=0 -k test_passwd
auditctl -w /etc/passwd -F success=1 -k test_passwd2
The second call to auditctl triggers the use-after-free, because
audit_to_parent updates krule->watch to use a previous existing watch
and drops the reference to the newly created watch.
To fix the issue, we grab a reference of watch and we release it at the
end of the function.
Signed-off-by: Ronny Chevalier <ronny.chevalier(a)hp.com>
---
v2:
- Move audit_get_watch before audit_find_parent. In the case of
audit_get_nd failing.
---
kernel/audit_watch.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index f1ba88994508..6d9b3f2bb1e2 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -430,6 +430,14 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
if (ret)
return ret;
+ /*
+ * When we will be calling audit_add_to_parent, krule->watch might have
+ * been updated and watch might have been freed.
+ * So we need to keep a reference of watch.
+ */
+
+ audit_get_watch(watch);
+
/* either find an old parent or attach a new one */
parent = audit_find_parent(d_backing_inode(parent_path.dentry));
if (!parent) {
@@ -446,6 +454,7 @@ int audit_add_watch(struct audit_krule *krule, struct list_head **list)
*list = &audit_inode_hash[h];
error:
path_put(&parent_path);
+ audit_put_watch(watch);
return ret;
}
--
2.17.1
