From ea956d8be91edc702a98b7fe1f9463e7ca8c42ab Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 10 Oct 2018 16:22:57 -0400 Subject: [PATCH 01/22] audit: print empty EXECVE args Empty executable arguments were being skipped when printing out the list of arguments in an EXECVE record, making it appear they were somehow lost. Include empty arguments as an itemized empty string. Reproducer: autrace /bin/ls "" "/etc" ausearch --start recent -m execve -i | grep EXECVE type=EXECVE msg=audit(10/03/2018 13:04:03.208:1391) : argc=3 a0=/bin/ls a2=/etc With fix: type=EXECVE msg=audit(10/03/2018 21:51:38.290:194) : argc=3 a0=/bin/ls a1= a2=/etc type=EXECVE msg=audit(1538617898.290:194): argc=3 a0="/bin/ls" a1="" a2="/etc" Passes audit-testsuite. GH issue tracker at https://github.com/linux-audit/audit-kernel/issues/99 Signed-off-by: Richard Guy Briggs [PM: cleaned up the commit metadata] Signed-off-by: Paul Moore --- kernel/auditsc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index b2d1f043f17fb9..1513873e23bd12 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1107,7 +1107,7 @@ static void audit_log_execve_info(struct audit_context *context, } /* write as much as we can to the audit log */ - if (len_buf > 0) { + if (len_buf >= 0) { /* NOTE: some magic numbers here - basically if we * can't fit a reasonable amount of data into the * existing audit buffer, flush it and start with From 9f16d2e6241b2fc664523f17d74adda7489f496b Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Wed, 17 Oct 2018 12:14:52 +0200 Subject: [PATCH 02/22] audit_tree: Remove mark->lock locking Currently, audit_tree code uses mark->lock to protect against detaching of mark from an inode. In most places it however also uses mark->group->mark_mutex (as we need to atomically replace attached marks) and this provides protection against mark detaching as well. So just remove protection with mark->lock from audit tree code and replace it with mark->group->mark_mutex protection in all the places. It simplifies the code and gets rid of some ugly catches like calling fsnotify_add_mark_locked() with mark->lock held (which cannot sleep only because we hold a reference to another mark attached to the same inode). Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index ea43181cde4a2d..1b55b1026a36a1 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -193,7 +193,7 @@ static inline struct list_head *chunk_hash(unsigned long key) return chunk_hash_heads + n % HASH_SIZE; } -/* hash_lock & entry->lock is held by caller */ +/* hash_lock & entry->group->mark_mutex is held by caller */ static void insert_hash(struct audit_chunk *chunk) { unsigned long key = chunk_to_key(chunk); @@ -256,13 +256,11 @@ static void untag_chunk(struct node *p) new = alloc_chunk(size); mutex_lock(&entry->group->mark_mutex); - spin_lock(&entry->lock); /* * mark_mutex protects mark from getting detached and thus also from * mark->connector->obj getting NULL. */ if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { - spin_unlock(&entry->lock); mutex_unlock(&entry->group->mark_mutex); if (new) fsnotify_put_mark(&new->mark); @@ -280,7 +278,6 @@ static void untag_chunk(struct node *p) list_del_init(&p->list); list_del_rcu(&chunk->hash); spin_unlock(&hash_lock); - spin_unlock(&entry->lock); mutex_unlock(&entry->group->mark_mutex); fsnotify_destroy_mark(entry, audit_tree_group); goto out; @@ -323,7 +320,6 @@ static void untag_chunk(struct node *p) list_for_each_entry(owner, &new->trees, same_root) owner->root = new; spin_unlock(&hash_lock); - spin_unlock(&entry->lock); mutex_unlock(&entry->group->mark_mutex); fsnotify_destroy_mark(entry, audit_tree_group); fsnotify_put_mark(&new->mark); /* drop initial reference */ @@ -340,7 +336,6 @@ static void untag_chunk(struct node *p) p->owner = NULL; put_tree(owner); spin_unlock(&hash_lock); - spin_unlock(&entry->lock); mutex_unlock(&entry->group->mark_mutex); out: fsnotify_put_mark(entry); @@ -360,12 +355,12 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) return -ENOSPC; } - spin_lock(&entry->lock); + mutex_lock(&entry->group->mark_mutex); spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - spin_unlock(&entry->lock); + mutex_unlock(&entry->group->mark_mutex); fsnotify_destroy_mark(entry, audit_tree_group); fsnotify_put_mark(entry); return 0; @@ -380,7 +375,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) } insert_hash(chunk); spin_unlock(&hash_lock); - spin_unlock(&entry->lock); + mutex_unlock(&entry->group->mark_mutex); fsnotify_put_mark(entry); /* drop initial reference */ return 0; } @@ -421,14 +416,12 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) chunk_entry = &chunk->mark; mutex_lock(&old_entry->group->mark_mutex); - spin_lock(&old_entry->lock); /* * mark_mutex protects mark from getting detached and thus also from * mark->connector->obj getting NULL. */ if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { /* old_entry is being shot, lets just lie */ - spin_unlock(&old_entry->lock); mutex_unlock(&old_entry->group->mark_mutex); fsnotify_put_mark(old_entry); fsnotify_put_mark(&chunk->mark); @@ -437,23 +430,16 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj, FSNOTIFY_OBJ_TYPE_INODE, 1)) { - spin_unlock(&old_entry->lock); mutex_unlock(&old_entry->group->mark_mutex); fsnotify_put_mark(chunk_entry); fsnotify_put_mark(old_entry); return -ENOSPC; } - /* even though we hold old_entry->lock, this is safe since chunk_entry->lock could NEVER have been grabbed before */ - spin_lock(&chunk_entry->lock); spin_lock(&hash_lock); - - /* we now hold old_entry->lock, chunk_entry->lock, and hash_lock */ if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - spin_unlock(&chunk_entry->lock); - spin_unlock(&old_entry->lock); mutex_unlock(&old_entry->group->mark_mutex); fsnotify_destroy_mark(chunk_entry, audit_tree_group); @@ -485,8 +471,6 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) list_add(&tree->same_root, &chunk->trees); } spin_unlock(&hash_lock); - spin_unlock(&chunk_entry->lock); - spin_unlock(&old_entry->lock); mutex_unlock(&old_entry->group->mark_mutex); fsnotify_destroy_mark(old_entry, audit_tree_group); fsnotify_put_mark(chunk_entry); /* drop initial reference */ From a5789b07b35aa56569dff762bfc063303a9ccb95 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 03/22] audit: Fix possible spurious -ENOSPC error When an inode is tagged with a tree, tag_chunk() checks whether there is audit_tree_group mark attached to the inode and adds one if not. However nothing protects another tag_chunk() to add the mark between we've checked and try to add the fsnotify mark thus resulting in an error from fsnotify_add_mark() and consequently an ENOSPC error from tag_chunk(). Fix the problem by holding mark_mutex over the whole check-insert code sequence. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 1b55b1026a36a1..8a74b468b6660d 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -342,25 +342,29 @@ static void untag_chunk(struct node *p) spin_lock(&hash_lock); } +/* Call with group->mark_mutex held, releases it */ static int create_chunk(struct inode *inode, struct audit_tree *tree) { struct fsnotify_mark *entry; struct audit_chunk *chunk = alloc_chunk(1); - if (!chunk) + + if (!chunk) { + mutex_unlock(&audit_tree_group->mark_mutex); return -ENOMEM; + } entry = &chunk->mark; - if (fsnotify_add_inode_mark(entry, inode, 0)) { + if (fsnotify_add_inode_mark_locked(entry, inode, 0)) { + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); return -ENOSPC; } - mutex_lock(&entry->group->mark_mutex); spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(entry, audit_tree_group); fsnotify_put_mark(entry); return 0; @@ -375,7 +379,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) } insert_hash(chunk); spin_unlock(&hash_lock); - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); /* drop initial reference */ return 0; } @@ -389,6 +393,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) struct node *p; int n; + mutex_lock(&audit_tree_group->mark_mutex); old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_tree_group); if (!old_entry) @@ -401,6 +406,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) for (n = 0; n < old->count; n++) { if (old->owners[n].owner == tree) { spin_unlock(&hash_lock); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); return 0; } @@ -409,20 +415,20 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) chunk = alloc_chunk(old->count + 1); if (!chunk) { + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); return -ENOMEM; } chunk_entry = &chunk->mark; - mutex_lock(&old_entry->group->mark_mutex); /* * mark_mutex protects mark from getting detached and thus also from * mark->connector->obj getting NULL. */ if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { /* old_entry is being shot, lets just lie */ - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); fsnotify_put_mark(&chunk->mark); return -ENOENT; @@ -430,7 +436,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj, FSNOTIFY_OBJ_TYPE_INODE, 1)) { - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(chunk_entry); fsnotify_put_mark(old_entry); return -ENOSPC; @@ -440,7 +446,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(chunk_entry, audit_tree_group); @@ -471,7 +477,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) list_add(&tree->same_root, &chunk->trees); } spin_unlock(&hash_lock); - mutex_unlock(&old_entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_destroy_mark(old_entry, audit_tree_group); fsnotify_put_mark(chunk_entry); /* drop initial reference */ fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */ From b1e4603b92d8aef8776e5673dc13fedb68d32ea4 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 04/22] audit: Fix possible tagging failures Audit tree code is replacing marks attached to inodes in non-atomic way. Thus fsnotify_find_mark() in tag_chunk() may find a mark that belongs to a chunk that is no longer valid one and will soon be destroyed. Tags added to such chunk will be simply lost. Fix the problem by making sure old mark is marked as going away (through fsnotify_detach_mark()) before dropping mark_mutex and thus in an atomic way wrt tag_chunk(). Note that this does not fix the problem completely as if tag_chunk() finds a mark that is going away, it fails with -ENOENT. But at least the failure is not silent and currently there's no way to search for another fsnotify mark attached to the inode. We'll fix this problem in later patch. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 8a74b468b6660d..c194dbd537532c 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -278,8 +278,9 @@ static void untag_chunk(struct node *p) list_del_init(&p->list); list_del_rcu(&chunk->hash); spin_unlock(&hash_lock); + fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); - fsnotify_destroy_mark(entry, audit_tree_group); + fsnotify_free_mark(entry); goto out; } @@ -320,8 +321,9 @@ static void untag_chunk(struct node *p) list_for_each_entry(owner, &new->trees, same_root) owner->root = new; spin_unlock(&hash_lock); + fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); - fsnotify_destroy_mark(entry, audit_tree_group); + fsnotify_free_mark(entry); fsnotify_put_mark(&new->mark); /* drop initial reference */ goto out; @@ -364,8 +366,9 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; + fsnotify_detach_mark(entry); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_destroy_mark(entry, audit_tree_group); + fsnotify_free_mark(entry); fsnotify_put_mark(entry); return 0; } @@ -446,10 +449,9 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (tree->goner) { spin_unlock(&hash_lock); chunk->dead = 1; + fsnotify_detach_mark(chunk_entry); mutex_unlock(&audit_tree_group->mark_mutex); - - fsnotify_destroy_mark(chunk_entry, audit_tree_group); - + fsnotify_free_mark(chunk_entry); fsnotify_put_mark(chunk_entry); fsnotify_put_mark(old_entry); return 0; @@ -477,8 +479,9 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) list_add(&tree->same_root, &chunk->trees); } spin_unlock(&hash_lock); + fsnotify_detach_mark(old_entry); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_destroy_mark(old_entry, audit_tree_group); + fsnotify_free_mark(old_entry); fsnotify_put_mark(chunk_entry); /* drop initial reference */ fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */ return 0; From 8d20d6e9301d7b3777d66d47dd5b89acd645cd39 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 05/22] audit: Embed key into chunk Currently chunk hash key (which is in fact pointer to the inode) is derived as chunk->mark.conn->obj. It is tricky to make this dereference reliable for hash table lookups only under RCU as mark can get detached from the connector and connector gets freed independently of the running lookup. Thus there is a possible use after free / NULL ptr dereference issue: CPU1 CPU2 untag_chunk() ... audit_tree_lookup() list_for_each_entry_rcu(p, list, hash) { list_del_rcu(&chunk->hash); fsnotify_destroy_mark(entry); fsnotify_put_mark(entry) chunk_to_key(p) if (!chunk->mark.connector) ... hlist_del_init_rcu(&mark->obj_list); if (hlist_empty(&conn->list)) { inode = fsnotify_detach_connector_from_object(conn); mark->connector = NULL; ... frees connector from workqueue chunk->mark.connector->obj This race is probably impossible to hit in practice as the race window on CPU1 is very narrow and CPU2 has a lot of code to execute. Still it's better to have this fixed. Since the inode the chunk is attached to is constant during chunk's lifetime it is easy to cache the key in the chunk itself and thus avoid these issues. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index c194dbd537532c..bac5dd90c62983 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -24,6 +24,7 @@ struct audit_tree { struct audit_chunk { struct list_head hash; + unsigned long key; struct fsnotify_mark mark; struct list_head trees; /* with root here */ int dead; @@ -172,21 +173,6 @@ static unsigned long inode_to_key(const struct inode *inode) return (unsigned long)&inode->i_fsnotify_marks; } -/* - * Function to return search key in our hash from chunk. Key 0 is special and - * should never be present in the hash. - */ -static unsigned long chunk_to_key(struct audit_chunk *chunk) -{ - /* - * We have a reference to the mark so it should be attached to a - * connector. - */ - if (WARN_ON_ONCE(!chunk->mark.connector)) - return 0; - return (unsigned long)chunk->mark.connector->obj; -} - static inline struct list_head *chunk_hash(unsigned long key) { unsigned long n = key / L1_CACHE_BYTES; @@ -196,12 +182,12 @@ static inline struct list_head *chunk_hash(unsigned long key) /* hash_lock & entry->group->mark_mutex is held by caller */ static void insert_hash(struct audit_chunk *chunk) { - unsigned long key = chunk_to_key(chunk); struct list_head *list; if (!(chunk->mark.flags & FSNOTIFY_MARK_FLAG_ATTACHED)) return; - list = chunk_hash(key); + WARN_ON_ONCE(!chunk->key); + list = chunk_hash(chunk->key); list_add_rcu(&chunk->hash, list); } @@ -213,7 +199,7 @@ struct audit_chunk *audit_tree_lookup(const struct inode *inode) struct audit_chunk *p; list_for_each_entry_rcu(p, list, hash) { - if (chunk_to_key(p) == key) { + if (p->key == key) { atomic_long_inc(&p->refs); return p; } @@ -295,6 +281,7 @@ static void untag_chunk(struct node *p) chunk->dead = 1; spin_lock(&hash_lock); + new->key = chunk->key; list_replace_init(&chunk->trees, &new->trees); if (owner->root == chunk) { list_del_init(&owner->same_root); @@ -380,6 +367,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) tree->root = chunk; list_add(&tree->same_root, &chunk->trees); } + chunk->key = inode_to_key(inode); insert_hash(chunk); spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); @@ -456,6 +444,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) fsnotify_put_mark(old_entry); return 0; } + chunk->key = old->key; list_replace_init(&old->trees, &chunk->trees); for (n = 0, p = chunk->owners; n < old->count; n++, p++) { struct audit_tree *s = old->owners[n].owner; @@ -654,7 +643,7 @@ void audit_trim_trees(void) /* this could be NULL if the watch is dying else where... */ node->index |= 1U<<31; if (iterate_mounts(compare_root, - (void *)chunk_to_key(chunk), + (void *)(chunk->key), root_mnt)) node->index &= ~(1U<<31); } From 1635e5722350597b6a149bdb131358fcd7e34906 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 06/22] audit: Make hash table insertion safe against concurrent lookups Currently, the audit tree code does not make sure that when a chunk is inserted into the hash table, it is fully initialized. So in theory a user of RCU lookup could see uninitialized structure in the hash table and crash. Add appropriate barriers between initialization of the structure and its insertion into hash table. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 32 +++++++++++++++++++++++++++++--- 1 file changed, 29 insertions(+), 3 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index bac5dd90c62983..307749d6773c14 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -186,6 +186,12 @@ static void insert_hash(struct audit_chunk *chunk) if (!(chunk->mark.flags & FSNOTIFY_MARK_FLAG_ATTACHED)) return; + /* + * Make sure chunk is fully initialized before making it visible in the + * hash. Pairs with a data dependency barrier in READ_ONCE() in + * audit_tree_lookup(). + */ + smp_wmb(); WARN_ON_ONCE(!chunk->key); list = chunk_hash(chunk->key); list_add_rcu(&chunk->hash, list); @@ -199,7 +205,11 @@ struct audit_chunk *audit_tree_lookup(const struct inode *inode) struct audit_chunk *p; list_for_each_entry_rcu(p, list, hash) { - if (p->key == key) { + /* + * We use a data dependency barrier in READ_ONCE() to make sure + * the chunk we see is fully initialized. + */ + if (READ_ONCE(p->key) == key) { atomic_long_inc(&p->refs); return p; } @@ -304,9 +314,15 @@ static void untag_chunk(struct node *p) list_replace_init(&chunk->owners[j].list, &new->owners[i].list); } - list_replace_rcu(&chunk->hash, &new->hash); list_for_each_entry(owner, &new->trees, same_root) owner->root = new; + /* + * Make sure chunk is fully initialized before making it visible in the + * hash. Pairs with a data dependency barrier in READ_ONCE() in + * audit_tree_lookup(). + */ + smp_wmb(); + list_replace_rcu(&chunk->hash, &new->hash); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); @@ -368,6 +384,10 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) list_add(&tree->same_root, &chunk->trees); } chunk->key = inode_to_key(inode); + /* + * Inserting into the hash table has to go last as once we do that RCU + * readers can see the chunk. + */ insert_hash(chunk); spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); @@ -459,7 +479,6 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) p->owner = tree; get_tree(tree); list_add(&p->list, &tree->chunks); - list_replace_rcu(&old->hash, &chunk->hash); list_for_each_entry(owner, &chunk->trees, same_root) owner->root = chunk; old->dead = 1; @@ -467,6 +486,13 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) tree->root = chunk; list_add(&tree->same_root, &chunk->trees); } + /* + * Make sure chunk is fully initialized before making it visible in the + * hash. Pairs with a data dependency barrier in READ_ONCE() in + * audit_tree_lookup(). + */ + smp_wmb(); + list_replace_rcu(&old->hash, &chunk->hash); spin_unlock(&hash_lock); fsnotify_detach_mark(old_entry); mutex_unlock(&audit_tree_group->mark_mutex); From d31b326d3ce7b1ff2ec36470dfcccb14a6c3e04e Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 07/22] audit: Factor out chunk replacement code Chunk replacement code is very similar for the cases where we grow or shrink chunk. Factor the code out into a common helper function. Signed-off-by: Jan Kara Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit_tree.c | 86 +++++++++++++++++++++------------------------ 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 307749d6773c14..d8f6cfa0005b49 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -235,6 +235,38 @@ static struct audit_chunk *find_chunk(struct node *p) return container_of(p, struct audit_chunk, owners[0]); } +static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old, + struct node *skip) +{ + struct audit_tree *owner; + int i, j; + + new->key = old->key; + list_splice_init(&old->trees, &new->trees); + list_for_each_entry(owner, &new->trees, same_root) + owner->root = new; + for (i = j = 0; j < old->count; i++, j++) { + if (&old->owners[j] == skip) { + i--; + continue; + } + owner = old->owners[j].owner; + new->owners[i].owner = owner; + new->owners[i].index = old->owners[j].index - j + i; + if (!owner) /* result of earlier fallback */ + continue; + get_tree(owner); + list_replace_init(&old->owners[j].list, &new->owners[i].list); + } + /* + * Make sure chunk is fully initialized before making it visible in the + * hash. Pairs with a data dependency barrier in READ_ONCE() in + * audit_tree_lookup(). + */ + smp_wmb(); + list_replace_rcu(&old->hash, &new->hash); +} + static void untag_chunk(struct node *p) { struct audit_chunk *chunk = find_chunk(p); @@ -242,7 +274,6 @@ static void untag_chunk(struct node *p) struct audit_chunk *new = NULL; struct audit_tree *owner; int size = chunk->count - 1; - int i, j; fsnotify_get_mark(entry); @@ -291,38 +322,16 @@ static void untag_chunk(struct node *p) chunk->dead = 1; spin_lock(&hash_lock); - new->key = chunk->key; - list_replace_init(&chunk->trees, &new->trees); if (owner->root == chunk) { list_del_init(&owner->same_root); owner->root = NULL; } - - for (i = j = 0; j <= size; i++, j++) { - struct audit_tree *s; - if (&chunk->owners[j] == p) { - list_del_init(&p->list); - i--; - continue; - } - s = chunk->owners[j].owner; - new->owners[i].owner = s; - new->owners[i].index = chunk->owners[j].index - j + i; - if (!s) /* result of earlier fallback */ - continue; - get_tree(s); - list_replace_init(&chunk->owners[j].list, &new->owners[i].list); - } - - list_for_each_entry(owner, &new->trees, same_root) - owner->root = new; + list_del_init(&p->list); /* - * Make sure chunk is fully initialized before making it visible in the - * hash. Pairs with a data dependency barrier in READ_ONCE() in - * audit_tree_lookup(). + * This has to go last when updating chunk as once replace_chunk() is + * called, new RCU readers can see the new chunk. */ - smp_wmb(); - list_replace_rcu(&chunk->hash, &new->hash); + replace_chunk(new, chunk, p); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); @@ -399,7 +408,6 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) static int tag_chunk(struct inode *inode, struct audit_tree *tree) { struct fsnotify_mark *old_entry, *chunk_entry; - struct audit_tree *owner; struct audit_chunk *chunk, *old; struct node *p; int n; @@ -464,35 +472,21 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) fsnotify_put_mark(old_entry); return 0; } - chunk->key = old->key; - list_replace_init(&old->trees, &chunk->trees); - for (n = 0, p = chunk->owners; n < old->count; n++, p++) { - struct audit_tree *s = old->owners[n].owner; - p->owner = s; - p->index = old->owners[n].index; - if (!s) /* result of fallback in untag */ - continue; - get_tree(s); - list_replace_init(&old->owners[n].list, &p->list); - } + p = &chunk->owners[chunk->count - 1]; p->index = (chunk->count - 1) | (1U<<31); p->owner = tree; get_tree(tree); list_add(&p->list, &tree->chunks); - list_for_each_entry(owner, &chunk->trees, same_root) - owner->root = chunk; old->dead = 1; if (!tree->root) { tree->root = chunk; list_add(&tree->same_root, &chunk->trees); } /* - * Make sure chunk is fully initialized before making it visible in the - * hash. Pairs with a data dependency barrier in READ_ONCE() in - * audit_tree_lookup(). + * This has to go last when updating chunk as once replace_chunk() is + * called, new RCU readers can see the new chunk. */ - smp_wmb(); - list_replace_rcu(&old->hash, &chunk->hash); + replace_chunk(chunk, old, NULL); spin_unlock(&hash_lock); fsnotify_detach_mark(old_entry); mutex_unlock(&audit_tree_group->mark_mutex); From 8cd0feb5234ccda3c15de35b40c8010a406dfc03 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:48 -0500 Subject: [PATCH 08/22] audit: Remove pointless check in insert_hash() The audit_tree_group->mark_mutex is held all the time while we create the fsnotify mark, add it to the inode, and insert chunk into the hash. Hence mark cannot get detached during this time and so the check whether the mark is attached in insert_hash() is pointless. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index d8f6cfa0005b49..d150514ff15ee0 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -184,8 +184,6 @@ static void insert_hash(struct audit_chunk *chunk) { struct list_head *list; - if (!(chunk->mark.flags & FSNOTIFY_MARK_FLAG_ATTACHED)) - return; /* * Make sure chunk is fully initialized before making it visible in the * hash. Pairs with a data dependency barrier in READ_ONCE() in From a8375713fb1ff28ec718b601895958f1db775774 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:49 -0500 Subject: [PATCH 09/22] audit: Provide helper for dropping mark's chunk reference Provide a helper function audit_mark_put_chunk() for dropping mark's reference (which has to happen only after RCU grace period expires). Currently that happens only from a single place but in later patches we introduce more callers. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index d150514ff15ee0..35c031ebcc12e4 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -132,10 +132,20 @@ static void __put_chunk(struct rcu_head *rcu) audit_put_chunk(chunk); } +/* + * Drop reference to the chunk that was held by the mark. This is the reference + * that gets dropped after we've removed the chunk from the hash table and we + * use it to make sure chunk cannot be freed before RCU grace period expires. + */ +static void audit_mark_put_chunk(struct audit_chunk *chunk) +{ + call_rcu(&chunk->head, __put_chunk); +} + static void audit_tree_destroy_watch(struct fsnotify_mark *entry) { struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark); - call_rcu(&chunk->head, __put_chunk); + audit_mark_put_chunk(chunk); } static struct audit_chunk *alloc_chunk(int count) From 5f5161300d7bd530e062428ac694824832960cf5 Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:49 -0500 Subject: [PATCH 10/22] audit: Allocate fsnotify mark independently of chunk Allocate fsnotify mark independently instead of embedding it inside chunk. This will allow us to just replace chunk attached to mark when growing / shrinking chunk instead of replacing mark attached to inode which is a more complex operation. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara Signed-off-by: Paul Moore --- kernel/audit_tree.c | 64 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 50 insertions(+), 14 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 35c031ebcc12e4..c98ab2d68a1c0d 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -25,7 +25,7 @@ struct audit_tree { struct audit_chunk { struct list_head hash; unsigned long key; - struct fsnotify_mark mark; + struct fsnotify_mark *mark; struct list_head trees; /* with root here */ int dead; int count; @@ -38,6 +38,11 @@ struct audit_chunk { } owners[]; }; +struct audit_tree_mark { + struct fsnotify_mark mark; + struct audit_chunk *chunk; +}; + static LIST_HEAD(tree_list); static LIST_HEAD(prune_list); static struct task_struct *prune_thread; @@ -73,6 +78,7 @@ static struct task_struct *prune_thread; */ static struct fsnotify_group *audit_tree_group; +static struct kmem_cache *audit_tree_mark_cachep __read_mostly; static struct audit_tree *alloc_tree(const char *s) { @@ -142,10 +148,33 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk) call_rcu(&chunk->head, __put_chunk); } +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry) +{ + return container_of(entry, struct audit_tree_mark, mark); +} + +static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark) +{ + return audit_mark(mark)->chunk; +} + static void audit_tree_destroy_watch(struct fsnotify_mark *entry) { - struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark); + struct audit_chunk *chunk = mark_chunk(entry); audit_mark_put_chunk(chunk); + kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry)); +} + +static struct fsnotify_mark *alloc_mark(void) +{ + struct audit_tree_mark *amark; + + amark = kmem_cache_zalloc(audit_tree_mark_cachep, GFP_KERNEL); + if (!amark) + return NULL; + fsnotify_init_mark(&amark->mark, audit_tree_group); + amark->mark.mask = FS_IN_IGNORED; + return &amark->mark; } static struct audit_chunk *alloc_chunk(int count) @@ -159,6 +188,13 @@ static struct audit_chunk *alloc_chunk(int count) if (!chunk) return NULL; + chunk->mark = alloc_mark(); + if (!chunk->mark) { + kfree(chunk); + return NULL; + } + audit_mark(chunk->mark)->chunk = chunk; + INIT_LIST_HEAD(&chunk->hash); INIT_LIST_HEAD(&chunk->trees); chunk->count = count; @@ -167,8 +203,6 @@ static struct audit_chunk *alloc_chunk(int count) INIT_LIST_HEAD(&chunk->owners[i].list); chunk->owners[i].index = i; } - fsnotify_init_mark(&chunk->mark, audit_tree_group); - chunk->mark.mask = FS_IN_IGNORED; return chunk; } @@ -278,7 +312,7 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old, static void untag_chunk(struct node *p) { struct audit_chunk *chunk = find_chunk(p); - struct fsnotify_mark *entry = &chunk->mark; + struct fsnotify_mark *entry = chunk->mark; struct audit_chunk *new = NULL; struct audit_tree *owner; int size = chunk->count - 1; @@ -298,7 +332,7 @@ static void untag_chunk(struct node *p) if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { mutex_unlock(&entry->group->mark_mutex); if (new) - fsnotify_put_mark(&new->mark); + fsnotify_put_mark(new->mark); goto out; } @@ -322,9 +356,9 @@ static void untag_chunk(struct node *p) if (!new) goto Fallback; - if (fsnotify_add_mark_locked(&new->mark, entry->connector->obj, + if (fsnotify_add_mark_locked(new->mark, entry->connector->obj, FSNOTIFY_OBJ_TYPE_INODE, 1)) { - fsnotify_put_mark(&new->mark); + fsnotify_put_mark(new->mark); goto Fallback; } @@ -344,7 +378,7 @@ static void untag_chunk(struct node *p) fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); fsnotify_free_mark(entry); - fsnotify_put_mark(&new->mark); /* drop initial reference */ + fsnotify_put_mark(new->mark); /* drop initial reference */ goto out; Fallback: @@ -375,7 +409,7 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) return -ENOMEM; } - entry = &chunk->mark; + entry = chunk->mark; if (fsnotify_add_inode_mark_locked(entry, inode, 0)) { mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); @@ -426,7 +460,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (!old_entry) return create_chunk(inode, tree); - old = container_of(old_entry, struct audit_chunk, mark); + old = mark_chunk(old_entry); /* are we already there? */ spin_lock(&hash_lock); @@ -447,7 +481,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) return -ENOMEM; } - chunk_entry = &chunk->mark; + chunk_entry = chunk->mark; /* * mark_mutex protects mark from getting detached and thus also from @@ -457,7 +491,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) /* old_entry is being shot, lets just lie */ mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(old_entry); - fsnotify_put_mark(&chunk->mark); + fsnotify_put_mark(chunk->mark); return -ENOENT; } @@ -1011,7 +1045,7 @@ static int audit_tree_handle_event(struct fsnotify_group *group, static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group) { - struct audit_chunk *chunk = container_of(entry, struct audit_chunk, mark); + struct audit_chunk *chunk = mark_chunk(entry); evict_chunk(chunk); @@ -1032,6 +1066,8 @@ static int __init audit_tree_init(void) { int i; + audit_tree_mark_cachep = KMEM_CACHE(audit_tree_mark, SLAB_PANIC); + audit_tree_group = fsnotify_alloc_group(&audit_tree_ops); if (IS_ERR(audit_tree_group)) audit_panic("cannot initialize fsnotify group for rectree watches"); From 49a4ee7d98dbe34cfed90b930664c8a9fa73b24c Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:49 -0500 Subject: [PATCH 11/22] audit: Guarantee forward progress of chunk untagging When removing chunk from a tree, we do shrink the chunk. This can fail for various reasons (due to races, ENOMEM, etc.) and in some cases we just bail from untag_chunk() relying on someone else to cleanup. Although this currently works, later we will need to add new failure situation which would break. Also this simplifies the code and will allow us to make locking around untag_chunk() less awkward. Signed-off-by: Jan Kara Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit_tree.c | 42 +++++++++++++++++------------------------- 1 file changed, 17 insertions(+), 25 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index c98ab2d68a1c0d..ca2b6baff7aa20 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -309,16 +309,28 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old, list_replace_rcu(&old->hash, &new->hash); } +static void remove_chunk_node(struct audit_chunk *chunk, struct node *p) +{ + struct audit_tree *owner = p->owner; + + if (owner->root == chunk) { + list_del_init(&owner->same_root); + owner->root = NULL; + } + list_del_init(&p->list); + p->owner = NULL; + put_tree(owner); +} + static void untag_chunk(struct node *p) { struct audit_chunk *chunk = find_chunk(p); struct fsnotify_mark *entry = chunk->mark; struct audit_chunk *new = NULL; - struct audit_tree *owner; int size = chunk->count - 1; + remove_chunk_node(chunk, p); fsnotify_get_mark(entry); - spin_unlock(&hash_lock); if (size) @@ -336,15 +348,10 @@ static void untag_chunk(struct node *p) goto out; } - owner = p->owner; - if (!size) { chunk->dead = 1; spin_lock(&hash_lock); list_del_init(&chunk->trees); - if (owner->root == chunk) - owner->root = NULL; - list_del_init(&p->list); list_del_rcu(&chunk->hash); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); @@ -354,21 +361,16 @@ static void untag_chunk(struct node *p) } if (!new) - goto Fallback; + goto out_mutex; if (fsnotify_add_mark_locked(new->mark, entry->connector->obj, FSNOTIFY_OBJ_TYPE_INODE, 1)) { fsnotify_put_mark(new->mark); - goto Fallback; + goto out_mutex; } chunk->dead = 1; spin_lock(&hash_lock); - if (owner->root == chunk) { - list_del_init(&owner->same_root); - owner->root = NULL; - } - list_del_init(&p->list); /* * This has to go last when updating chunk as once replace_chunk() is * called, new RCU readers can see the new chunk. @@ -381,17 +383,7 @@ static void untag_chunk(struct node *p) fsnotify_put_mark(new->mark); /* drop initial reference */ goto out; -Fallback: - // do the best we can - spin_lock(&hash_lock); - if (owner->root == chunk) { - list_del_init(&owner->same_root); - owner->root = NULL; - } - list_del_init(&p->list); - p->owner = NULL; - put_tree(owner); - spin_unlock(&hash_lock); +out_mutex: mutex_unlock(&entry->group->mark_mutex); out: fsnotify_put_mark(entry); From c22fcde775dcc9f46d73d694061441efdc7bdaad Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:49 -0500 Subject: [PATCH 12/22] audit: Drop all unused chunk nodes during deletion When deleting chunk from a tree, drop all unused nodes in a chunk instead of just the one used by the tree. This gets rid of possibly lingering unused nodes (created due to fallback path in untag_chunk()) and also removes some special cases and will allow us to simplify locking in untag_chunk(). Signed-off-by: Jan Kara Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit_tree.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index ca2b6baff7aa20..145e8c92dd31b5 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -277,8 +277,7 @@ static struct audit_chunk *find_chunk(struct node *p) return container_of(p, struct audit_chunk, owners[0]); } -static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old, - struct node *skip) +static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old) { struct audit_tree *owner; int i, j; @@ -288,7 +287,7 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old, list_for_each_entry(owner, &new->trees, same_root) owner->root = new; for (i = j = 0; j < old->count; i++, j++) { - if (&old->owners[j] == skip) { + if (!old->owners[j].owner) { i--; continue; } @@ -322,20 +321,28 @@ static void remove_chunk_node(struct audit_chunk *chunk, struct node *p) put_tree(owner); } +static int chunk_count_trees(struct audit_chunk *chunk) +{ + int i; + int ret = 0; + + for (i = 0; i < chunk->count; i++) + if (chunk->owners[i].owner) + ret++; + return ret; +} + static void untag_chunk(struct node *p) { struct audit_chunk *chunk = find_chunk(p); struct fsnotify_mark *entry = chunk->mark; struct audit_chunk *new = NULL; - int size = chunk->count - 1; + int size; remove_chunk_node(chunk, p); fsnotify_get_mark(entry); spin_unlock(&hash_lock); - if (size) - new = alloc_chunk(size); - mutex_lock(&entry->group->mark_mutex); /* * mark_mutex protects mark from getting detached and thus also from @@ -348,6 +355,7 @@ static void untag_chunk(struct node *p) goto out; } + size = chunk_count_trees(chunk); if (!size) { chunk->dead = 1; spin_lock(&hash_lock); @@ -360,6 +368,7 @@ static void untag_chunk(struct node *p) goto out; } + new = alloc_chunk(size); if (!new) goto out_mutex; @@ -375,7 +384,7 @@ static void untag_chunk(struct node *p) * This has to go last when updating chunk as once replace_chunk() is * called, new RCU readers can see the new chunk. */ - replace_chunk(new, chunk, p); + replace_chunk(new, chunk); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); mutex_unlock(&entry->group->mark_mutex); @@ -520,7 +529,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) * This has to go last when updating chunk as once replace_chunk() is * called, new RCU readers can see the new chunk. */ - replace_chunk(chunk, old, NULL); + replace_chunk(chunk, old); spin_unlock(&hash_lock); fsnotify_detach_mark(old_entry); mutex_unlock(&audit_tree_group->mark_mutex); From 8432c70062978d9a57bde6715496d585ec520c3e Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:54:56 -0500 Subject: [PATCH 13/22] audit: Simplify locking around untag_chunk() untag_chunk() has to be called with hash_lock, it drops it and reacquires it when returning. The unlocking of hash_lock is thus hidden from the callers of untag_chunk() with is rather error prone. Reorganize the code so that untag_chunk() is called without hash_lock, only with mark reference preventing the chunk from going away. Since this requires some more code in the caller of untag_chunk() to assure forward progress, factor out loop pruning tree from all chunks into a common helper function. Signed-off-by: Jan Kara Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit_tree.c | 77 +++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 145e8c92dd31b5..82b27da7031c7d 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -332,28 +332,18 @@ static int chunk_count_trees(struct audit_chunk *chunk) return ret; } -static void untag_chunk(struct node *p) +static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) { - struct audit_chunk *chunk = find_chunk(p); - struct fsnotify_mark *entry = chunk->mark; - struct audit_chunk *new = NULL; + struct audit_chunk *new; int size; - remove_chunk_node(chunk, p); - fsnotify_get_mark(entry); - spin_unlock(&hash_lock); - - mutex_lock(&entry->group->mark_mutex); + mutex_lock(&audit_tree_group->mark_mutex); /* * mark_mutex protects mark from getting detached and thus also from * mark->connector->obj getting NULL. */ - if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { - mutex_unlock(&entry->group->mark_mutex); - if (new) - fsnotify_put_mark(new->mark); - goto out; - } + if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) + goto out_mutex; size = chunk_count_trees(chunk); if (!size) { @@ -363,9 +353,9 @@ static void untag_chunk(struct node *p) list_del_rcu(&chunk->hash); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_free_mark(entry); - goto out; + return; } new = alloc_chunk(size); @@ -387,16 +377,13 @@ static void untag_chunk(struct node *p) replace_chunk(new, chunk); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_free_mark(entry); fsnotify_put_mark(new->mark); /* drop initial reference */ - goto out; + return; out_mutex: - mutex_unlock(&entry->group->mark_mutex); -out: - fsnotify_put_mark(entry); - spin_lock(&hash_lock); + mutex_unlock(&audit_tree_group->mark_mutex); } /* Call with group->mark_mutex held, releases it */ @@ -579,22 +566,45 @@ static void kill_rules(struct audit_tree *tree) } /* - * finish killing struct audit_tree + * Remove tree from chunks. If 'tagged' is set, remove tree only from tagged + * chunks. The function expects tagged chunks are all at the beginning of the + * chunks list. */ -static void prune_one(struct audit_tree *victim) +static void prune_tree_chunks(struct audit_tree *victim, bool tagged) { spin_lock(&hash_lock); while (!list_empty(&victim->chunks)) { struct node *p; + struct audit_chunk *chunk; + struct fsnotify_mark *mark; + + p = list_first_entry(&victim->chunks, struct node, list); + /* have we run out of marked? */ + if (tagged && !(p->index & (1U<<31))) + break; + chunk = find_chunk(p); + mark = chunk->mark; + remove_chunk_node(chunk, p); + fsnotify_get_mark(mark); + spin_unlock(&hash_lock); - p = list_entry(victim->chunks.next, struct node, list); + untag_chunk(chunk, mark); + fsnotify_put_mark(mark); - untag_chunk(p); + spin_lock(&hash_lock); } spin_unlock(&hash_lock); put_tree(victim); } +/* + * finish killing struct audit_tree + */ +static void prune_one(struct audit_tree *victim) +{ + prune_tree_chunks(victim, false); +} + /* trim the uncommitted chunks from tree */ static void trim_marked(struct audit_tree *tree) @@ -614,18 +624,11 @@ static void trim_marked(struct audit_tree *tree) list_add(p, &tree->chunks); } } + spin_unlock(&hash_lock); - while (!list_empty(&tree->chunks)) { - struct node *node; - - node = list_entry(tree->chunks.next, struct node, list); - - /* have we run out of marked? */ - if (!(node->index & (1U<<31))) - break; + prune_tree_chunks(tree, true); - untag_chunk(node); - } + spin_lock(&hash_lock); if (!tree->root && !tree->goner) { tree->goner = 1; spin_unlock(&hash_lock); From 83d23bc8aedc51fc40078026e9fae6e349d83b2a Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:55:16 -0500 Subject: [PATCH 14/22] audit: Replace chunk attached to mark instead of replacing mark Audit tree code currently associates new fsnotify mark with each new chunk. As chunk attached to an inode is replaced when new tag is added / removed, we also need to remove old fsnotify mark and add a new one on such occasion. This is cumbersome and makes locking rules somewhat difficult to follow. Fix these problems by allocating fsnotify mark independently of chunk and keeping it all the time while there is some chunk attached to an inode. Also add documentation about the locking rules so that things are easier to follow. Signed-off-by: Jan Kara Reviewed-by: Richard Guy Briggs [PM: minor merge fuzz due to updated patches previously in the series] Signed-off-by: Paul Moore --- kernel/audit_tree.c | 160 +++++++++++++++++++++++--------------------- 1 file changed, 83 insertions(+), 77 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 82b27da7031c7d..1d8dc20296fb72 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -27,7 +27,6 @@ struct audit_chunk { unsigned long key; struct fsnotify_mark *mark; struct list_head trees; /* with root here */ - int dead; int count; atomic_long_t refs; struct rcu_head head; @@ -48,8 +47,15 @@ static LIST_HEAD(prune_list); static struct task_struct *prune_thread; /* - * One struct chunk is attached to each inode of interest. - * We replace struct chunk on tagging/untagging. + * One struct chunk is attached to each inode of interest through + * audit_tree_mark (fsnotify mark). We replace struct chunk on tagging / + * untagging, the mark is stable as long as there is chunk attached. The + * association between mark and chunk is protected by hash_lock and + * audit_tree_group->mark_mutex. Thus as long as we hold + * audit_tree_group->mark_mutex and check that the mark is alive by + * FSNOTIFY_MARK_FLAG_ATTACHED flag check, we are sure the mark points to + * the current chunk. + * * Rules have pointer to struct audit_tree. * Rules have struct list_head rlist forming a list of rules over * the same tree. @@ -68,8 +74,12 @@ static struct task_struct *prune_thread; * tree is refcounted; one reference for "some rules on rules_list refer to * it", one for each chunk with pointer to it. * - * chunk is refcounted by embedded fsnotify_mark + .refs (non-zero refcount - * of watch contributes 1 to .refs). + * chunk is refcounted by embedded .refs. Mark associated with the chunk holds + * one chunk reference. This reference is dropped either when a mark is going + * to be freed (corresponding inode goes away) or when chunk attached to the + * mark gets replaced. This reference must be dropped using + * audit_mark_put_chunk() to make sure the reference is dropped only after RCU + * grace period as it protects RCU readers of the hash table. * * node.index allows to get from node.list to containing chunk. * MSB of that sucker is stolen to mark taggings that we might have to @@ -160,8 +170,6 @@ static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark) static void audit_tree_destroy_watch(struct fsnotify_mark *entry) { - struct audit_chunk *chunk = mark_chunk(entry); - audit_mark_put_chunk(chunk); kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry)); } @@ -188,13 +196,6 @@ static struct audit_chunk *alloc_chunk(int count) if (!chunk) return NULL; - chunk->mark = alloc_mark(); - if (!chunk->mark) { - kfree(chunk); - return NULL; - } - audit_mark(chunk->mark)->chunk = chunk; - INIT_LIST_HEAD(&chunk->hash); INIT_LIST_HEAD(&chunk->trees); chunk->count = count; @@ -277,6 +278,20 @@ static struct audit_chunk *find_chunk(struct node *p) return container_of(p, struct audit_chunk, owners[0]); } +static void replace_mark_chunk(struct fsnotify_mark *entry, + struct audit_chunk *chunk) +{ + struct audit_chunk *old; + + assert_spin_locked(&hash_lock); + old = mark_chunk(entry); + audit_mark(entry)->chunk = chunk; + if (chunk) + chunk->mark = entry; + if (old) + old->mark = NULL; +} + static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old) { struct audit_tree *owner; @@ -299,6 +314,7 @@ static void replace_chunk(struct audit_chunk *new, struct audit_chunk *old) get_tree(owner); list_replace_init(&old->owners[j].list, &new->owners[i].list); } + replace_mark_chunk(old->mark, new); /* * Make sure chunk is fully initialized before making it visible in the * hash. Pairs with a data dependency barrier in READ_ONCE() in @@ -339,21 +355,23 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) mutex_lock(&audit_tree_group->mark_mutex); /* - * mark_mutex protects mark from getting detached and thus also from - * mark->connector->obj getting NULL. + * mark_mutex stabilizes chunk attached to the mark so we can check + * whether it didn't change while we've dropped hash_lock. */ - if (chunk->dead || !(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) + if (!(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED) || + mark_chunk(entry) != chunk) goto out_mutex; size = chunk_count_trees(chunk); if (!size) { - chunk->dead = 1; spin_lock(&hash_lock); list_del_init(&chunk->trees); list_del_rcu(&chunk->hash); + replace_mark_chunk(entry, NULL); spin_unlock(&hash_lock); fsnotify_detach_mark(entry); mutex_unlock(&audit_tree_group->mark_mutex); + audit_mark_put_chunk(chunk); fsnotify_free_mark(entry); return; } @@ -362,13 +380,6 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) if (!new) goto out_mutex; - if (fsnotify_add_mark_locked(new->mark, entry->connector->obj, - FSNOTIFY_OBJ_TYPE_INODE, 1)) { - fsnotify_put_mark(new->mark); - goto out_mutex; - } - - chunk->dead = 1; spin_lock(&hash_lock); /* * This has to go last when updating chunk as once replace_chunk() is @@ -376,10 +387,8 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) */ replace_chunk(new, chunk); spin_unlock(&hash_lock); - fsnotify_detach_mark(entry); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_free_mark(entry); - fsnotify_put_mark(new->mark); /* drop initial reference */ + audit_mark_put_chunk(chunk); return; out_mutex: @@ -397,23 +406,31 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) return -ENOMEM; } - entry = chunk->mark; + entry = alloc_mark(); + if (!entry) { + mutex_unlock(&audit_tree_group->mark_mutex); + kfree(chunk); + return -ENOMEM; + } + if (fsnotify_add_inode_mark_locked(entry, inode, 0)) { mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_put_mark(entry); + kfree(chunk); return -ENOSPC; } spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); - chunk->dead = 1; fsnotify_detach_mark(entry); mutex_unlock(&audit_tree_group->mark_mutex); fsnotify_free_mark(entry); fsnotify_put_mark(entry); + kfree(chunk); return 0; } + replace_mark_chunk(entry, chunk); chunk->owners[0].index = (1U << 31); chunk->owners[0].owner = tree; get_tree(tree); @@ -430,33 +447,41 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) insert_hash(chunk); spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); /* drop initial reference */ + /* + * Drop our initial reference. When mark we point to is getting freed, + * we get notification through ->freeing_mark callback and cleanup + * chunk pointing to this mark. + */ + fsnotify_put_mark(entry); return 0; } /* the first tagged inode becomes root of tree */ static int tag_chunk(struct inode *inode, struct audit_tree *tree) { - struct fsnotify_mark *old_entry, *chunk_entry; + struct fsnotify_mark *entry; struct audit_chunk *chunk, *old; struct node *p; int n; mutex_lock(&audit_tree_group->mark_mutex); - old_entry = fsnotify_find_mark(&inode->i_fsnotify_marks, - audit_tree_group); - if (!old_entry) + entry = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_tree_group); + if (!entry) return create_chunk(inode, tree); - old = mark_chunk(old_entry); - + /* + * Found mark is guaranteed to be attached and mark_mutex protects mark + * from getting detached and thus it makes sure there is chunk attached + * to the mark. + */ /* are we already there? */ spin_lock(&hash_lock); + old = mark_chunk(entry); for (n = 0; n < old->count; n++) { if (old->owners[n].owner == tree) { spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(old_entry); + fsnotify_put_mark(entry); return 0; } } @@ -465,41 +490,16 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) chunk = alloc_chunk(old->count + 1); if (!chunk) { mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(old_entry); + fsnotify_put_mark(entry); return -ENOMEM; } - chunk_entry = chunk->mark; - - /* - * mark_mutex protects mark from getting detached and thus also from - * mark->connector->obj getting NULL. - */ - if (!(old_entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED)) { - /* old_entry is being shot, lets just lie */ - mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(old_entry); - fsnotify_put_mark(chunk->mark); - return -ENOENT; - } - - if (fsnotify_add_mark_locked(chunk_entry, old_entry->connector->obj, - FSNOTIFY_OBJ_TYPE_INODE, 1)) { - mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(chunk_entry); - fsnotify_put_mark(old_entry); - return -ENOSPC; - } - spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); - chunk->dead = 1; - fsnotify_detach_mark(chunk_entry); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_free_mark(chunk_entry); - fsnotify_put_mark(chunk_entry); - fsnotify_put_mark(old_entry); + fsnotify_put_mark(entry); + kfree(chunk); return 0; } p = &chunk->owners[chunk->count - 1]; @@ -507,7 +507,6 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) p->owner = tree; get_tree(tree); list_add(&p->list, &tree->chunks); - old->dead = 1; if (!tree->root) { tree->root = chunk; list_add(&tree->same_root, &chunk->trees); @@ -518,11 +517,10 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) */ replace_chunk(chunk, old); spin_unlock(&hash_lock); - fsnotify_detach_mark(old_entry); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_free_mark(old_entry); - fsnotify_put_mark(chunk_entry); /* drop initial reference */ - fsnotify_put_mark(old_entry); /* pair to fsnotify_find mark_entry */ + fsnotify_put_mark(entry); /* pair to fsnotify_find mark_entry */ + audit_mark_put_chunk(old); + return 0; } @@ -585,6 +583,9 @@ static void prune_tree_chunks(struct audit_tree *victim, bool tagged) chunk = find_chunk(p); mark = chunk->mark; remove_chunk_node(chunk, p); + /* Racing with audit_tree_freeing_mark()? */ + if (!mark) + continue; fsnotify_get_mark(mark); spin_unlock(&hash_lock); @@ -1007,10 +1008,6 @@ static void evict_chunk(struct audit_chunk *chunk) int need_prune = 0; int n; - if (chunk->dead) - return; - - chunk->dead = 1; mutex_lock(&audit_filter_mutex); spin_lock(&hash_lock); while (!list_empty(&chunk->trees)) { @@ -1049,9 +1046,18 @@ static int audit_tree_handle_event(struct fsnotify_group *group, static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group) { - struct audit_chunk *chunk = mark_chunk(entry); + struct audit_chunk *chunk; - evict_chunk(chunk); + mutex_lock(&entry->group->mark_mutex); + spin_lock(&hash_lock); + chunk = mark_chunk(entry); + replace_mark_chunk(entry, NULL); + spin_unlock(&hash_lock); + mutex_unlock(&entry->group->mark_mutex); + if (chunk) { + evict_chunk(chunk); + audit_mark_put_chunk(chunk); + } /* * We are guaranteed to have at least one reference to the mark from From f905c2fc3980a41aeccb8673ab10ed5e616391fd Mon Sep 17 00:00:00 2001 From: Jan Kara Date: Mon, 12 Nov 2018 09:55:16 -0500 Subject: [PATCH 15/22] audit: Use 'mark' name for fsnotify_mark variables Variables pointing to fsnotify_mark are sometimes called 'entry' and sometimes 'mark'. Use 'mark' in all places. Reviewed-by: Richard Guy Briggs Signed-off-by: Jan Kara [PM: minor merge fuzz due to updated patches previously in the series] Signed-off-by: Paul Moore --- kernel/audit_tree.c | 79 +++++++++++++++++++++++---------------------- 1 file changed, 40 insertions(+), 39 deletions(-) diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 1d8dc20296fb72..58e84eb5d8265f 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -158,9 +158,9 @@ static void audit_mark_put_chunk(struct audit_chunk *chunk) call_rcu(&chunk->head, __put_chunk); } -static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *entry) +static inline struct audit_tree_mark *audit_mark(struct fsnotify_mark *mark) { - return container_of(entry, struct audit_tree_mark, mark); + return container_of(mark, struct audit_tree_mark, mark); } static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark) @@ -168,9 +168,9 @@ static struct audit_chunk *mark_chunk(struct fsnotify_mark *mark) return audit_mark(mark)->chunk; } -static void audit_tree_destroy_watch(struct fsnotify_mark *entry) +static void audit_tree_destroy_watch(struct fsnotify_mark *mark) { - kmem_cache_free(audit_tree_mark_cachep, audit_mark(entry)); + kmem_cache_free(audit_tree_mark_cachep, audit_mark(mark)); } static struct fsnotify_mark *alloc_mark(void) @@ -224,7 +224,7 @@ static inline struct list_head *chunk_hash(unsigned long key) return chunk_hash_heads + n % HASH_SIZE; } -/* hash_lock & entry->group->mark_mutex is held by caller */ +/* hash_lock & mark->group->mark_mutex is held by caller */ static void insert_hash(struct audit_chunk *chunk) { struct list_head *list; @@ -278,16 +278,16 @@ static struct audit_chunk *find_chunk(struct node *p) return container_of(p, struct audit_chunk, owners[0]); } -static void replace_mark_chunk(struct fsnotify_mark *entry, +static void replace_mark_chunk(struct fsnotify_mark *mark, struct audit_chunk *chunk) { struct audit_chunk *old; assert_spin_locked(&hash_lock); - old = mark_chunk(entry); - audit_mark(entry)->chunk = chunk; + old = mark_chunk(mark); + audit_mark(mark)->chunk = chunk; if (chunk) - chunk->mark = entry; + chunk->mark = mark; if (old) old->mark = NULL; } @@ -348,7 +348,7 @@ static int chunk_count_trees(struct audit_chunk *chunk) return ret; } -static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) +static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *mark) { struct audit_chunk *new; int size; @@ -358,8 +358,8 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) * mark_mutex stabilizes chunk attached to the mark so we can check * whether it didn't change while we've dropped hash_lock. */ - if (!(entry->flags & FSNOTIFY_MARK_FLAG_ATTACHED) || - mark_chunk(entry) != chunk) + if (!(mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) || + mark_chunk(mark) != chunk) goto out_mutex; size = chunk_count_trees(chunk); @@ -367,12 +367,12 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) spin_lock(&hash_lock); list_del_init(&chunk->trees); list_del_rcu(&chunk->hash); - replace_mark_chunk(entry, NULL); + replace_mark_chunk(mark, NULL); spin_unlock(&hash_lock); - fsnotify_detach_mark(entry); + fsnotify_detach_mark(mark); mutex_unlock(&audit_tree_group->mark_mutex); audit_mark_put_chunk(chunk); - fsnotify_free_mark(entry); + fsnotify_free_mark(mark); return; } @@ -398,7 +398,7 @@ static void untag_chunk(struct audit_chunk *chunk, struct fsnotify_mark *entry) /* Call with group->mark_mutex held, releases it */ static int create_chunk(struct inode *inode, struct audit_tree *tree) { - struct fsnotify_mark *entry; + struct fsnotify_mark *mark; struct audit_chunk *chunk = alloc_chunk(1); if (!chunk) { @@ -406,16 +406,16 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) return -ENOMEM; } - entry = alloc_mark(); - if (!entry) { + mark = alloc_mark(); + if (!mark) { mutex_unlock(&audit_tree_group->mark_mutex); kfree(chunk); return -ENOMEM; } - if (fsnotify_add_inode_mark_locked(entry, inode, 0)) { + if (fsnotify_add_inode_mark_locked(mark, inode, 0)) { mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); + fsnotify_put_mark(mark); kfree(chunk); return -ENOSPC; } @@ -423,14 +423,14 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) spin_lock(&hash_lock); if (tree->goner) { spin_unlock(&hash_lock); - fsnotify_detach_mark(entry); + fsnotify_detach_mark(mark); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_free_mark(entry); - fsnotify_put_mark(entry); + fsnotify_free_mark(mark); + fsnotify_put_mark(mark); kfree(chunk); return 0; } - replace_mark_chunk(entry, chunk); + replace_mark_chunk(mark, chunk); chunk->owners[0].index = (1U << 31); chunk->owners[0].owner = tree; get_tree(tree); @@ -452,21 +452,21 @@ static int create_chunk(struct inode *inode, struct audit_tree *tree) * we get notification through ->freeing_mark callback and cleanup * chunk pointing to this mark. */ - fsnotify_put_mark(entry); + fsnotify_put_mark(mark); return 0; } /* the first tagged inode becomes root of tree */ static int tag_chunk(struct inode *inode, struct audit_tree *tree) { - struct fsnotify_mark *entry; + struct fsnotify_mark *mark; struct audit_chunk *chunk, *old; struct node *p; int n; mutex_lock(&audit_tree_group->mark_mutex); - entry = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_tree_group); - if (!entry) + mark = fsnotify_find_mark(&inode->i_fsnotify_marks, audit_tree_group); + if (!mark) return create_chunk(inode, tree); /* @@ -476,12 +476,12 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) */ /* are we already there? */ spin_lock(&hash_lock); - old = mark_chunk(entry); + old = mark_chunk(mark); for (n = 0; n < old->count; n++) { if (old->owners[n].owner == tree) { spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); + fsnotify_put_mark(mark); return 0; } } @@ -490,7 +490,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) chunk = alloc_chunk(old->count + 1); if (!chunk) { mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); + fsnotify_put_mark(mark); return -ENOMEM; } @@ -498,7 +498,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) if (tree->goner) { spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); + fsnotify_put_mark(mark); kfree(chunk); return 0; } @@ -518,7 +518,7 @@ static int tag_chunk(struct inode *inode, struct audit_tree *tree) replace_chunk(chunk, old); spin_unlock(&hash_lock); mutex_unlock(&audit_tree_group->mark_mutex); - fsnotify_put_mark(entry); /* pair to fsnotify_find mark_entry */ + fsnotify_put_mark(mark); /* pair to fsnotify_find_mark */ audit_mark_put_chunk(old); return 0; @@ -1044,16 +1044,17 @@ static int audit_tree_handle_event(struct fsnotify_group *group, return 0; } -static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify_group *group) +static void audit_tree_freeing_mark(struct fsnotify_mark *mark, + struct fsnotify_group *group) { struct audit_chunk *chunk; - mutex_lock(&entry->group->mark_mutex); + mutex_lock(&mark->group->mark_mutex); spin_lock(&hash_lock); - chunk = mark_chunk(entry); - replace_mark_chunk(entry, NULL); + chunk = mark_chunk(mark); + replace_mark_chunk(mark, NULL); spin_unlock(&hash_lock); - mutex_unlock(&entry->group->mark_mutex); + mutex_unlock(&mark->group->mark_mutex); if (chunk) { evict_chunk(chunk); audit_mark_put_chunk(chunk); @@ -1063,7 +1064,7 @@ static void audit_tree_freeing_mark(struct fsnotify_mark *entry, struct fsnotify * We are guaranteed to have at least one reference to the mark from * either the inode or the caller of fsnotify_destroy_mark(). */ - BUG_ON(refcount_read(&entry->refcnt) < 1); + BUG_ON(refcount_read(&mark->refcnt) < 1); } static const struct fsnotify_ops audit_tree_ops = { From 0fe3c7fceb500de2d0adfb9dcf292580cd43ea38 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Fri, 16 Nov 2018 12:16:35 -0500 Subject: [PATCH 16/22] audit: localize audit_log_session_info prototype The audit_log_session_info() function is only used in kernel/audit*, so move its prototype to kernel/audit.h Signed-off-by: Richard Guy Briggs Signed-off-by: Paul Moore --- include/linux/audit.h | 2 -- kernel/audit.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index 9334fbef7bae63..58cf665f597e00 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -115,8 +115,6 @@ extern int audit_classify_compat_syscall(int abi, unsigned syscall); struct filename; -extern void audit_log_session_info(struct audit_buffer *ab); - #define AUDIT_OFF 0 #define AUDIT_ON 1 #define AUDIT_LOCKED 2 diff --git a/kernel/audit.h b/kernel/audit.h index 214e1494837060..9a3828bd387b24 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -210,6 +210,8 @@ struct audit_context { extern bool audit_ever_enabled; +extern void audit_log_session_info(struct audit_buffer *ab); + extern void audit_copy_inode(struct audit_names *name, const struct dentry *dentry, struct inode *inode); From a2c97da11cdb973b752dd434aee9636ce10ee737 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Fri, 16 Nov 2018 16:30:10 -0500 Subject: [PATCH 17/22] audit: use session_info helper There are still a couple of places (mark and watch config changes) that open code auid and ses fields in sequence in records instead of using the audit_log_session_info() helper. Use the helper. Adjust the helper to accommodate being the first fields. Passes audit-testsuite. Signed-off-by: Richard Guy Briggs [PM: fixed misspellings in the description] Signed-off-by: Paul Moore --- kernel/audit.c | 6 +++--- kernel/audit_fsnotify.c | 5 ++--- kernel/audit_watch.c | 5 ++--- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 2a8058764aa624..6c53e373b828ca 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -400,7 +400,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old, ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); if (unlikely(!ab)) return rc; - audit_log_format(ab, "%s=%u old=%u", function_name, new, old); + audit_log_format(ab, "%s=%u old=%u ", function_name, new, old); audit_log_session_info(ab); rc = audit_log_task_context(ab); if (rc) @@ -1067,7 +1067,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) *ab = audit_log_start(NULL, GFP_KERNEL, msg_type); if (unlikely(!*ab)) return; - audit_log_format(*ab, "pid=%d uid=%u", pid, uid); + audit_log_format(*ab, "pid=%d uid=%u ", pid, uid); audit_log_session_info(*ab); audit_log_task_context(*ab); } @@ -2042,7 +2042,7 @@ void audit_log_session_info(struct audit_buffer *ab) unsigned int sessionid = audit_get_sessionid(current); uid_t auid = from_kuid(&init_user_ns, audit_get_loginuid(current)); - audit_log_format(ab, " auid=%u ses=%u", auid, sessionid); + audit_log_format(ab, "auid=%u ses=%u", auid, sessionid); } void audit_log_key(struct audit_buffer *ab, char *key) diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index fba78047fb37c5..f90ffa699e5b8f 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -130,9 +130,8 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE); if (unlikely(!ab)) return; - audit_log_format(ab, "auid=%u ses=%u op=%s", - from_kuid(&init_user_ns, audit_get_loginuid(current)), - audit_get_sessionid(current), op); + audit_log_session_info(ab); + audit_log_format(ab, " op=%s", op); audit_log_format(ab, " path="); audit_log_untrustedstring(ab, audit_mark->path); audit_log_key(ab, rule->filterkey); diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c index 787c7afdf8294e..568e48d1d0ab78 100644 --- a/kernel/audit_watch.c +++ b/kernel/audit_watch.c @@ -245,9 +245,8 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc ab = audit_log_start(NULL, GFP_NOFS, AUDIT_CONFIG_CHANGE); if (!ab) return; - audit_log_format(ab, "auid=%u ses=%u op=%s", - from_kuid(&init_user_ns, audit_get_loginuid(current)), - audit_get_sessionid(current), op); + audit_log_session_info(ab); + audit_log_format(ab, "op=%s", op); audit_log_format(ab, " path="); audit_log_untrustedstring(ab, w->path); audit_log_key(ab, r->filterkey); From c8fc5d49c341805fee7fc295f2ea8a709f78aec4 Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Fri, 16 Nov 2018 12:17:35 -0500 Subject: [PATCH 18/22] audit: remove WATCH and TREE config options Remove the CONFIG_AUDIT_WATCH and CONFIG_AUDIT_TREE config options since they are both dependent on CONFIG_AUDITSYSCALL and force CONFIG_FSNOTIFY. Signed-off-by: Richard Guy Briggs Signed-off-by: Paul Moore --- init/Kconfig | 9 --------- kernel/Makefile | 4 +--- kernel/audit.h | 6 +++--- kernel/auditsc.c | 10 ---------- 4 files changed, 4 insertions(+), 25 deletions(-) diff --git a/init/Kconfig b/init/Kconfig index a4112e95724a05..7eb2538e6ca08b 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -335,15 +335,6 @@ config HAVE_ARCH_AUDITSYSCALL config AUDITSYSCALL def_bool y depends on AUDIT && HAVE_ARCH_AUDITSYSCALL - -config AUDIT_WATCH - def_bool y - depends on AUDITSYSCALL - select FSNOTIFY - -config AUDIT_TREE - def_bool y - depends on AUDITSYSCALL select FSNOTIFY source "kernel/irq/Kconfig" diff --git a/kernel/Makefile b/kernel/Makefile index 7343b3a9bff07d..9dc7f519129d8e 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -76,9 +76,7 @@ obj-$(CONFIG_IKCONFIG) += configs.o obj-$(CONFIG_SMP) += stop_machine.o obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o obj-$(CONFIG_AUDIT) += audit.o auditfilter.o -obj-$(CONFIG_AUDITSYSCALL) += auditsc.o -obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_fsnotify.o -obj-$(CONFIG_AUDIT_TREE) += audit_tree.o +obj-$(CONFIG_AUDITSYSCALL) += auditsc.o audit_watch.o audit_fsnotify.o audit_tree.o obj-$(CONFIG_GCOV_KERNEL) += gcov/ obj-$(CONFIG_KCOV) += kcov.o obj-$(CONFIG_KPROBES) += kprobes.o diff --git a/kernel/audit.h b/kernel/audit.h index 9a3828bd387b24..0b5295aeaebb13 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -268,7 +268,7 @@ extern struct tty_struct *audit_get_tty(struct task_struct *tsk); extern void audit_put_tty(struct tty_struct *tty); /* audit watch functions */ -#ifdef CONFIG_AUDIT_WATCH +#ifdef CONFIG_AUDITSYSCALL extern void audit_put_watch(struct audit_watch *watch); extern void audit_get_watch(struct audit_watch *watch); extern int audit_to_watch(struct audit_krule *krule, char *path, int len, u32 op); @@ -301,9 +301,9 @@ extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark #define audit_mark_compare(m, i, d) 0 #define audit_exe_compare(t, m) (-EINVAL) #define audit_dupe_exe(n, o) (-EINVAL) -#endif /* CONFIG_AUDIT_WATCH */ +#endif /* CONFIG_AUDITSYSCALL */ -#ifdef CONFIG_AUDIT_TREE +#ifdef CONFIG_AUDITSYSCALL extern struct audit_chunk *audit_tree_lookup(const struct inode *inode); extern void audit_put_chunk(struct audit_chunk *chunk); extern bool audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree); diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 1513873e23bd12..605f2d8252044d 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -200,7 +200,6 @@ static int audit_match_filetype(struct audit_context *ctx, int val) * References in it _are_ dropped - at the same time we free/drop aux stuff. */ -#ifdef CONFIG_AUDIT_TREE static void audit_set_auditable(struct audit_context *ctx) { if (!ctx->prio) { @@ -245,12 +244,10 @@ static int grow_tree_refs(struct audit_context *ctx) ctx->tree_count = 31; return 1; } -#endif static void unroll_tree_refs(struct audit_context *ctx, struct audit_tree_refs *p, int count) { -#ifdef CONFIG_AUDIT_TREE struct audit_tree_refs *q; int n; if (!p) { @@ -274,7 +271,6 @@ static void unroll_tree_refs(struct audit_context *ctx, } ctx->trees = p; ctx->tree_count = count; -#endif } static void free_tree_refs(struct audit_context *ctx) @@ -288,7 +284,6 @@ static void free_tree_refs(struct audit_context *ctx) static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) { -#ifdef CONFIG_AUDIT_TREE struct audit_tree_refs *p; int n; if (!tree) @@ -305,7 +300,6 @@ static int match_tree_refs(struct audit_context *ctx, struct audit_tree *tree) if (audit_tree_match(p->c[n], tree)) return 1; } -#endif return 0; } @@ -1602,7 +1596,6 @@ void __audit_syscall_exit(int success, long return_code) static inline void handle_one(const struct inode *inode) { -#ifdef CONFIG_AUDIT_TREE struct audit_context *context; struct audit_tree_refs *p; struct audit_chunk *chunk; @@ -1627,12 +1620,10 @@ static inline void handle_one(const struct inode *inode) return; } put_tree_ref(context, chunk); -#endif } static void handle_path(const struct dentry *dentry) { -#ifdef CONFIG_AUDIT_TREE struct audit_context *context; struct audit_tree_refs *p; const struct dentry *d, *parent; @@ -1685,7 +1676,6 @@ static void handle_path(const struct dentry *dentry) return; } rcu_read_unlock(); -#endif } static struct audit_names *audit_alloc_name(struct audit_context *context, From d0a3f18a70f2d9700bf9f5e9c4a505472388a7c1 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Thu, 2 Aug 2018 17:56:50 -0400 Subject: [PATCH 19/22] audit: minimize our use of audit_log_format() There are some cases where we are making multiple audit_log_format() calls in a row, for no apparent reason. Squash these down to a single audit_log_format() call whenever possible. Acked-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.c | 11 +++++------ kernel/audit_fsnotify.c | 3 +-- kernel/audit_tree.c | 3 +-- kernel/audit_watch.c | 3 +-- kernel/auditsc.c | 7 +++---- 5 files changed, 11 insertions(+), 16 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 6c53e373b828ca..d09298d3c2d2b6 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -2177,22 +2177,21 @@ void audit_log_name(struct audit_context *context, struct audit_names *n, } /* log the audit_names record type */ - audit_log_format(ab, " nametype="); switch(n->type) { case AUDIT_TYPE_NORMAL: - audit_log_format(ab, "NORMAL"); + audit_log_format(ab, " nametype=NORMAL"); break; case AUDIT_TYPE_PARENT: - audit_log_format(ab, "PARENT"); + audit_log_format(ab, " nametype=PARENT"); break; case AUDIT_TYPE_CHILD_DELETE: - audit_log_format(ab, "DELETE"); + audit_log_format(ab, " nametype=DELETE"); break; case AUDIT_TYPE_CHILD_CREATE: - audit_log_format(ab, "CREATE"); + audit_log_format(ab, " nametype=CREATE"); break; default: - audit_log_format(ab, "UNKNOWN"); + audit_log_format(ab, " nametype=UNKNOWN"); break; } diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index f90ffa699e5b8f..cf4512a3367583 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -131,8 +131,7 @@ static void audit_mark_log_rule_change(struct audit_fsnotify_mark *audit_mark, c if (unlikely(!ab)) return; audit_log_session_info(ab); - audit_log_format(ab, " op=%s", op); - audit_log_format(ab, " path="); + audit_log_format(ab, " op=%s path=", op); audit_log_untrustedstring(ab, audit_mark->path); audit_log_key(ab, rule->filterkey); audit_log_format(ab, " list=%d res=1", rule->listnr); diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 58e84eb5d8265f..d4af4d97f847a8 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -533,8 +533,7 @@ static void audit_tree_log_remove_rule(struct audit_krule *rule) ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); if (unlikely(!ab)) return; - audit_log_format(ab, "op=remove_rule"); - audit_log_format(ab, " dir="); + audit_log_format(ab, "op=remove_rule dir="); audit_log_untrustedstring(ab, rule->tree->pathname); audit_log_key(ab, rule->filterkey); audit_log_format(ab, " list=%d res=1", rule->listnr); diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c index 568e48d1d0ab78..20ef9ba134b0e7 100644 --- a/kernel/audit_watch.c +++ b/kernel/audit_watch.c @@ -246,8 +246,7 @@ static void audit_watch_log_rule_change(struct audit_krule *r, struct audit_watc if (!ab) return; audit_log_session_info(ab); - audit_log_format(ab, "op=%s", op); - audit_log_format(ab, " path="); + audit_log_format(ab, "op=%s path=", op); audit_log_untrustedstring(ab, w->path); audit_log_key(ab, r->filterkey); audit_log_format(ab, " list=%d res=1", r->listnr); diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 605f2d8252044d..51e735aedf5877 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2503,10 +2503,9 @@ void audit_seccomp_actions_logged(const char *names, const char *old_names, if (unlikely(!ab)) return; - audit_log_format(ab, "op=seccomp-logging"); - audit_log_format(ab, " actions=%s", names); - audit_log_format(ab, " old-actions=%s", old_names); - audit_log_format(ab, " res=%d", res); + audit_log_format(ab, + "op=seccomp-logging actions=%s old-actions=%s res=%d", + names, old_names, res); audit_log_end(ab); } From 2a1fe215e7300c7ebd6a7a24afcab71db5107bb0 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Mon, 26 Nov 2018 18:40:07 -0500 Subject: [PATCH 20/22] audit: use current whenever possible There are many places, notably audit_log_task_info() and audit_log_exit(), that take task_struct pointers but in reality they are always working on the current task. This patch eliminates the task_struct arguments and uses current directly which allows a number of cleanups as well. Acked-by: Richard Guy Briggs Signed-off-by: Paul Moore --- drivers/tty/tty_audit.c | 13 ++- include/linux/audit.h | 6 +- kernel/audit.c | 34 ++++---- kernel/audit.h | 2 +- kernel/auditsc.c | 131 +++++++++++++++---------------- security/integrity/ima/ima_api.c | 2 +- 6 files changed, 90 insertions(+), 98 deletions(-) diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c index 50f567b6a66ee4..28f87fd6a28e0b 100644 --- a/drivers/tty/tty_audit.c +++ b/drivers/tty/tty_audit.c @@ -61,20 +61,19 @@ static void tty_audit_log(const char *description, dev_t dev, unsigned char *data, size_t size) { struct audit_buffer *ab; - struct task_struct *tsk = current; - pid_t pid = task_pid_nr(tsk); - uid_t uid = from_kuid(&init_user_ns, task_uid(tsk)); - uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk)); - unsigned int sessionid = audit_get_sessionid(tsk); + pid_t pid = task_pid_nr(current); + uid_t uid = from_kuid(&init_user_ns, task_uid(current)); + uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(current)); + unsigned int sessionid = audit_get_sessionid(current); ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY); if (ab) { - char name[sizeof(tsk->comm)]; + char name[sizeof(current->comm)]; audit_log_format(ab, "%s pid=%u uid=%u auid=%u ses=%u major=%d" " minor=%d comm=", description, pid, uid, loginuid, sessionid, MAJOR(dev), MINOR(dev)); - get_task_comm(name, tsk); + get_task_comm(name, current); audit_log_untrustedstring(ab, name); audit_log_format(ab, " data="); audit_log_n_hex(ab, data, size); diff --git a/include/linux/audit.h b/include/linux/audit.h index 58cf665f597e00..a625c29a2ea2aa 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -151,8 +151,7 @@ extern void audit_log_link_denied(const char *operation); extern void audit_log_lost(const char *message); extern int audit_log_task_context(struct audit_buffer *ab); -extern void audit_log_task_info(struct audit_buffer *ab, - struct task_struct *tsk); +extern void audit_log_task_info(struct audit_buffer *ab); extern int audit_update_lsm_rules(void); @@ -200,8 +199,7 @@ static inline int audit_log_task_context(struct audit_buffer *ab) { return 0; } -static inline void audit_log_task_info(struct audit_buffer *ab, - struct task_struct *tsk) +static inline void audit_log_task_info(struct audit_buffer *ab) { } #define audit_enabled AUDIT_OFF #endif /* CONFIG_AUDIT */ diff --git a/kernel/audit.c b/kernel/audit.c index d09298d3c2d2b6..779671883349c1 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1096,10 +1096,11 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature if (audit_enabled == AUDIT_OFF) return; + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_FEATURE_CHANGE); if (!ab) return; - audit_log_task_info(ab, current); + audit_log_task_info(ab); audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d", audit_feature_names[which], !!old_feature, !!new_feature, !!old_lock, !!new_lock, res); @@ -2246,15 +2247,15 @@ void audit_log_d_path_exe(struct audit_buffer *ab, audit_log_format(ab, " exe=(null)"); } -struct tty_struct *audit_get_tty(struct task_struct *tsk) +struct tty_struct *audit_get_tty(void) { struct tty_struct *tty = NULL; unsigned long flags; - spin_lock_irqsave(&tsk->sighand->siglock, flags); - if (tsk->signal) - tty = tty_kref_get(tsk->signal->tty); - spin_unlock_irqrestore(&tsk->sighand->siglock, flags); + spin_lock_irqsave(¤t->sighand->siglock, flags); + if (current->signal) + tty = tty_kref_get(current->signal->tty); + spin_unlock_irqrestore(¤t->sighand->siglock, flags); return tty; } @@ -2263,25 +2264,24 @@ void audit_put_tty(struct tty_struct *tty) tty_kref_put(tty); } -void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) +void audit_log_task_info(struct audit_buffer *ab) { const struct cred *cred; - char comm[sizeof(tsk->comm)]; + char comm[sizeof(current->comm)]; struct tty_struct *tty; if (!ab) return; - /* tsk == current */ cred = current_cred(); - tty = audit_get_tty(tsk); + tty = audit_get_tty(); audit_log_format(ab, " ppid=%d pid=%d auid=%u uid=%u gid=%u" " euid=%u suid=%u fsuid=%u" " egid=%u sgid=%u fsgid=%u tty=%s ses=%u", - task_ppid_nr(tsk), - task_tgid_nr(tsk), - from_kuid(&init_user_ns, audit_get_loginuid(tsk)), + task_ppid_nr(current), + task_tgid_nr(current), + from_kuid(&init_user_ns, audit_get_loginuid(current)), from_kuid(&init_user_ns, cred->uid), from_kgid(&init_user_ns, cred->gid), from_kuid(&init_user_ns, cred->euid), @@ -2291,11 +2291,11 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk) from_kgid(&init_user_ns, cred->sgid), from_kgid(&init_user_ns, cred->fsgid), tty ? tty_name(tty) : "(none)", - audit_get_sessionid(tsk)); + audit_get_sessionid(current)); audit_put_tty(tty); audit_log_format(ab, " comm="); - audit_log_untrustedstring(ab, get_task_comm(comm, tsk)); - audit_log_d_path_exe(ab, tsk->mm); + audit_log_untrustedstring(ab, get_task_comm(comm, current)); + audit_log_d_path_exe(ab, current->mm); audit_log_task_context(ab); } EXPORT_SYMBOL(audit_log_task_info); @@ -2316,7 +2316,7 @@ void audit_log_link_denied(const char *operation) if (!ab) return; audit_log_format(ab, "op=%s", operation); - audit_log_task_info(ab, current); + audit_log_task_info(ab); audit_log_format(ab, " res=0"); audit_log_end(ab); } diff --git a/kernel/audit.h b/kernel/audit.h index 0b5295aeaebb13..91421679a16889 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -264,7 +264,7 @@ extern struct audit_entry *audit_dupe_rule(struct audit_krule *old); extern void audit_log_d_path_exe(struct audit_buffer *ab, struct mm_struct *mm); -extern struct tty_struct *audit_get_tty(struct task_struct *tsk); +extern struct tty_struct *audit_get_tty(void); extern void audit_put_tty(struct tty_struct *tty); /* audit watch functions */ diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 51e735aedf5877..6593a5207fb03f 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -830,44 +830,6 @@ void audit_filter_inodes(struct task_struct *tsk, struct audit_context *ctx) rcu_read_unlock(); } -/* Transfer the audit context pointer to the caller, clearing it in the tsk's struct */ -static inline struct audit_context *audit_take_context(struct task_struct *tsk, - int return_valid, - long return_code) -{ - struct audit_context *context = tsk->audit_context; - - if (!context) - return NULL; - context->return_valid = return_valid; - - /* - * we need to fix up the return code in the audit logs if the actual - * return codes are later going to be fixed up by the arch specific - * signal handlers - * - * This is actually a test for: - * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) || - * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK) - * - * but is faster than a bunch of || - */ - if (unlikely(return_code <= -ERESTARTSYS) && - (return_code >= -ERESTART_RESTARTBLOCK) && - (return_code != -ENOIOCTLCMD)) - context->return_code = -EINTR; - else - context->return_code = return_code; - - if (context->in_syscall && !context->dummy) { - audit_filter_syscall(tsk, context, &audit_filter_list[AUDIT_FILTER_EXIT]); - audit_filter_inodes(tsk, context); - } - - audit_set_context(tsk, NULL); - return context; -} - static inline void audit_proctitle_free(struct audit_context *context) { kfree(context->proctitle.value); @@ -1296,15 +1258,18 @@ static inline int audit_proctitle_rtrim(char *proctitle, int len) return len; } -static void audit_log_proctitle(struct task_struct *tsk, - struct audit_context *context) +static void audit_log_proctitle(void) { int res; char *buf; char *msg = "(null)"; int len = strlen(msg); + struct audit_context *context = audit_context(); struct audit_buffer *ab; + if (!context || context->dummy) + return; + ab = audit_log_start(context, GFP_KERNEL, AUDIT_PROCTITLE); if (!ab) return; /* audit_panic or being filtered */ @@ -1317,7 +1282,7 @@ static void audit_log_proctitle(struct task_struct *tsk, if (!buf) goto out; /* Historically called this from procfs naming */ - res = get_cmdline(tsk, buf, MAX_PROCTITLE_AUDIT_LEN); + res = get_cmdline(current, buf, MAX_PROCTITLE_AUDIT_LEN); if (res == 0) { kfree(buf); goto out; @@ -1337,15 +1302,15 @@ static void audit_log_proctitle(struct task_struct *tsk, audit_log_end(ab); } -static void audit_log_exit(struct audit_context *context, struct task_struct *tsk) +static void audit_log_exit(void) { int i, call_panic = 0; + struct audit_context *context = audit_context(); struct audit_buffer *ab; struct audit_aux_data *aux; struct audit_names *n; - /* tsk == current */ - context->personality = tsk->personality; + context->personality = current->personality; ab = audit_log_start(context, GFP_KERNEL, AUDIT_SYSCALL); if (!ab) @@ -1367,7 +1332,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts context->argv[3], context->name_count); - audit_log_task_info(ab, tsk); + audit_log_task_info(ab); audit_log_key(ab, context->filterkey); audit_log_end(ab); @@ -1456,7 +1421,7 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts audit_log_name(context, n, NULL, i++, &call_panic); } - audit_log_proctitle(tsk, context); + audit_log_proctitle(); /* Send end of event record to help user space know we are finished */ ab = audit_log_start(context, GFP_KERNEL, AUDIT_EOE); @@ -1474,22 +1439,31 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts */ void __audit_free(struct task_struct *tsk) { - struct audit_context *context; + struct audit_context *context = tsk->audit_context; - context = audit_take_context(tsk, 0, 0); if (!context) return; - /* Check for system calls that do not go through the exit - * function (e.g., exit_group), then free context block. - * We use GFP_ATOMIC here because we might be doing this - * in the context of the idle thread */ - /* that can happen only if we are called from do_exit() */ - if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) - audit_log_exit(context, tsk); + /* We are called either by do_exit() or the fork() error handling code; + * in the former case tsk == current and in the latter tsk is a + * random task_struct that doesn't doesn't have any meaningful data we + * need to log via audit_log_exit(). + */ + if (tsk == current && !context->dummy && context->in_syscall) { + context->return_valid = 0; + context->return_code = 0; + + audit_filter_syscall(tsk, context, + &audit_filter_list[AUDIT_FILTER_EXIT]); + audit_filter_inodes(tsk, context); + if (context->current_state == AUDIT_RECORD_CONTEXT) + audit_log_exit(); + } + if (!list_empty(&context->killed_trees)) audit_kill_trees(&context->killed_trees); + audit_set_context(tsk, NULL); audit_free_context(context); } @@ -1559,17 +1533,40 @@ void __audit_syscall_exit(int success, long return_code) { struct audit_context *context; - if (success) - success = AUDITSC_SUCCESS; - else - success = AUDITSC_FAILURE; - - context = audit_take_context(current, success, return_code); + context = audit_context(); if (!context) return; - if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT) - audit_log_exit(context, current); + if (!context->dummy && context->in_syscall) { + if (success) + context->return_valid = AUDITSC_SUCCESS; + else + context->return_valid = AUDITSC_FAILURE; + + /* + * we need to fix up the return code in the audit logs if the + * actual return codes are later going to be fixed up by the + * arch specific signal handlers + * + * This is actually a test for: + * (rc == ERESTARTSYS ) || (rc == ERESTARTNOINTR) || + * (rc == ERESTARTNOHAND) || (rc == ERESTART_RESTARTBLOCK) + * + * but is faster than a bunch of || + */ + if (unlikely(return_code <= -ERESTARTSYS) && + (return_code >= -ERESTART_RESTARTBLOCK) && + (return_code != -ENOIOCTLCMD)) + context->return_code = -EINTR; + else + context->return_code = return_code; + + audit_filter_syscall(current, context, + &audit_filter_list[AUDIT_FILTER_EXIT]); + audit_filter_inodes(current, context); + if (context->current_state == AUDIT_RECORD_CONTEXT) + audit_log_exit(); + } context->in_syscall = 0; context->prio = context->state == AUDIT_RECORD_CONTEXT ? ~0ULL : 0; @@ -1591,7 +1588,6 @@ void __audit_syscall_exit(int success, long return_code) kfree(context->filterkey); context->filterkey = NULL; } - audit_set_context(current, context); } static inline void handle_one(const struct inode *inode) @@ -2025,7 +2021,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid, uid = from_kuid(&init_user_ns, task_uid(current)); oldloginuid = from_kuid(&init_user_ns, koldloginuid); loginuid = from_kuid(&init_user_ns, kloginuid), - tty = audit_get_tty(current); + tty = audit_get_tty(); audit_log_format(ab, "pid=%d uid=%u", task_tgid_nr(current), uid); audit_log_task_context(ab); @@ -2046,7 +2042,6 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid, */ int audit_set_loginuid(kuid_t loginuid) { - struct task_struct *task = current; unsigned int oldsessionid, sessionid = AUDIT_SID_UNSET; kuid_t oldloginuid; int rc; @@ -2065,8 +2060,8 @@ int audit_set_loginuid(kuid_t loginuid) sessionid = (unsigned int)atomic_inc_return(&session_id); } - task->sessionid = sessionid; - task->loginuid = loginuid; + current->sessionid = sessionid; + current->loginuid = loginuid; out: audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc); return rc; diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c index 99dd1d53fc35be..af134588ab4e8d 100644 --- a/security/integrity/ima/ima_api.c +++ b/security/integrity/ima/ima_api.c @@ -336,7 +336,7 @@ void ima_audit_measurement(struct integrity_iint_cache *iint, audit_log_untrustedstring(ab, filename); audit_log_format(ab, " hash=\"%s:%s\"", algo_name, hash); - audit_log_task_info(ab, current); + audit_log_task_info(ab); audit_log_end(ab); iint->flags |= IMA_AUDITED; From 9a547c7e575fc2501c12081558fda3027d0f2a5e Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Fri, 30 Nov 2018 16:13:16 -0500 Subject: [PATCH 21/22] audit: shorten PATH cap values when zero Since the vast majority of files (99.993% on a typical system) have no fcaps, display "0" instead of the full zero-padded 16 hex digits in the two PATH record cap_f* fields to save netlink bandwidth and disk space. Simply changing the format to %x won't work since the value is two (or possibly more in the future) 32-bit hexadecimal values concatenated and bits in higher order values will be misrepresented. Passes audit-testsuite and userspace tools already work fine. Please see the github issue tracker for more details https://github.com/linux-audit/audit-kernel/issues/101 Signed-off-by: Richard Guy Briggs Acked-by: Steve Grubb Signed-off-by: Paul Moore --- kernel/audit.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/kernel/audit.c b/kernel/audit.c index 779671883349c1..a0a4544e69cadd 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -2059,11 +2059,13 @@ void audit_log_cap(struct audit_buffer *ab, char *prefix, kernel_cap_t *cap) { int i; - audit_log_format(ab, " %s=", prefix); - CAP_FOR_EACH_U32(i) { - audit_log_format(ab, "%08x", - cap->cap[CAP_LAST_U32 - i]); + if (cap_isclear(*cap)) { + audit_log_format(ab, " %s=0", prefix); + return; } + audit_log_format(ab, " %s=", prefix); + CAP_FOR_EACH_U32(i) + audit_log_format(ab, "%08x", cap->cap[CAP_LAST_U32 - i]); } static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name) From d406db524c32ca35bd85cada28a547fff3115715 Mon Sep 17 00:00:00 2001 From: YueHaibing Date: Sun, 9 Dec 2018 14:25:02 +0800 Subject: [PATCH 22/22] audit: remove duplicated include from audit.c Remove duplicated include. Signed-off-by: YueHaibing Signed-off-by: Paul Moore --- kernel/audit.c | 1 - 1 file changed, 1 deletion(-) diff --git a/kernel/audit.c b/kernel/audit.c index a0a4544e69cadd..632d360595560b 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -60,7 +60,6 @@ #include #include #include -#include #include