thanks for the poke, I have been meaning to get back to this
On 10/18/23 13:03, Paul Moore wrote:
On Mon, Oct 9, 2023 at 1:51 PM Paul Moore <p...@paul-moore.com> wrote:
On Mon, Oct 9, 2023 at 1:41 PM John Johansen
<john.johan...@canonical.com> wrote:
On 10/9/23 10:06, Paul Moore wrote:
I don't think anyone is objecting to resolving this, it's more a
matter of *how* we can resolve it.
currently I am see four crazy/stupid paths forward, each with their own
pain points.
...
4. caching a reference in the audit_context as paul has suggested.
I don't like this idea, but I'm struggling to come up with something
less awful. Below is a quick, untested patch to describe the concept
with code. It is worth noting that we don't take a mm_struct
reference in the io_uring entry point because I'm not sure filtering
on the executable file makes much sense there given the async nature
of io_uring, however I'm open to comments here (as well as pretty much
everything else in this pseudo-patch).
Looking at this a bit more, I'm now wondering if there is a fifth
option: call mmget() directly and skip the task_lock().
Take a look at the move_pages(2) code path:
SYSCALL_DEFINE6(move_pages, ...)
-> kernel_move_pages(...)
-> find_mm_struct(...)
-> mmget(...)
In find_mm_struct(), if the task being manipulated is *not* the
current task then get_task_mm() is called, which takes task_lock().
However, if the task being manipulated *is* the current task then the
task_lock() can be avoided and a direct call to get_mm() is used;
get_mm() does a simple atomic_inc() without any additional locking.
What do you think of this approach (untested, copy-n-pasted patch):
hrmmm, I like the idea but the task != current path still suffers from
the issue. If we can verify this case never happens great, otherwise
we either bail on that case or still need to come up with an
alternative.
otherwise it looks good
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 65075f1e4ac8..ffd17ad97324 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -526,8 +526,19 @@ int audit_exe_compare(struct task_struct *tsk, struct audit
_fsnotify_mark *mark)
struct file *exe_file;
unsigned long ino;
dev_t dev;
+ struct mm_struct *mm;
- exe_file = get_task_exe_file(tsk);
+ /* almost always (always?) comparing @current, but handle both cases */
+ if (likely(tsk == current)) {
+ mmget(current->mm);
+ mm = current->mm;
+ } else {
+ mm = get_task_mm(tsk);
+ if (!mm)
+ return 0;
+ }
+ exe_file = get_mm_exe_file(mm);> + mmput(mm);
if (!exe_file)
return 0;
ino = file_inode(exe_file)->i_ino;
--
paul-moore.com
