On 9/22/24 7:50 PM, Al Viro wrote:
> On Sun, Sep 22, 2024 at 01:49:01AM +0100, Al Viro wrote:
>
>> Another fun bit is that both audit_inode() and audit_inode_child()
>> may bump the refcount on struct filename. Which can get really fishy
>> if they get called by helper thread while the originator is exiting the
>> syscall - putname() from audit_free_names() in originator vs. refcount
>> increment in helper is Not Nice(tm), what with the refcount not being
>> atomic.
>
> *blink*
>
> OK, I really wonder which version had I been reading at the time; refcount
> is, indeed, atomic these days.
>
> Other problems (->aname pointing to other thread's struct audit_names
> and outliving reuse of those, as well as insane behaviour of audit predicates
> on symlink(2)) are, unfortunately, quite real - on the current mainline.
Traveling but took a quick look. As far as I can tell, for the "reuse
someone elses aname", we could do either:
1) Just don't reuse the entry. Then we can drop the struct
filename->aname completely as well. Yes that might incur an extra
alloc for the odd case of audit_enabled and being deep enough that
the preallocated names have been used, but doesn't anyone really
care? It'll be noise in the overhead anyway. Side note - that would
unalign struct filename again. Would be nice to drop audit_names from
a core fs struct...
2) Add a ref to struct audit_names, RCU kfree it when it drops to zero.
This would mean dropping struct audit_context->preallocated_names, as
otherwise we'd run into trouble there if a context gets blown away
while someone else has a ref to that audit_names struct. We could do
this without a ref as well, as long as we can store an audit_context
pointer in struct audit_names and be able to validate it under RCU.
If ctx doesn't match, don't use it.
And probably other ways too, those were just the two immediate ones I
thought it. Seems like option 1 is simpler and just fine? Quick hack:
diff --git a/fs/namei.c b/fs/namei.c
index 891b169e38c9..11263f779b96 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -206,7 +206,6 @@ getname_flags(const char __user *filename, int flags)
atomic_set(&result->refcnt, 1);
result->uptr = filename;
- result->aname = NULL;
audit_getname(result);
return result;
}
@@ -254,7 +253,6 @@ getname_kernel(const char * filename)
}
memcpy((char *)result->name, filename, len);
result->uptr = NULL;
- result->aname = NULL;
atomic_set(&result->refcnt, 1);
audit_getname(result);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0df3e5f0dd2b..859244c877b4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2685,10 +2685,8 @@ struct filename {
const char *name; /* pointer to actual string */
const __user char *uptr; /* original userland pointer */
atomic_t refcnt;
- struct audit_names *aname;
const char iname[];
};
-static_assert(offsetof(struct filename, iname) % sizeof(long) == 0);
static inline struct mnt_idmap *file_mnt_idmap(const struct file *file)
{
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cd57053b4a69..09caf8408225 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2240,7 +2240,6 @@ void __audit_getname(struct filename *name)
n->name = name;
n->name_len = AUDIT_NAME_FULL;
- name->aname = n;
atomic_inc(&name->refcnt);
}
@@ -2325,22 +2324,6 @@ void __audit_inode(struct filename *name, const struct dentry *dentry,
if (!name)
goto out_alloc;
- /*
- * If we have a pointer to an audit_names entry already, then we can
- * just use it directly if the type is correct.
- */
- n = name->aname;
- if (n) {
- if (parent) {
- if (n->type == AUDIT_TYPE_PARENT ||
- n->type == AUDIT_TYPE_UNKNOWN)
- goto out;
- } else {
- if (n->type != AUDIT_TYPE_PARENT)
- goto out;
- }
- }
-
list_for_each_entry_reverse(n, &context->names_list, list) {
if (n->ino) {
/* valid inode number, use that for the comparison */
--
Jens Axboe
