| Message ID | 20231024183828.209525-2-paul@paul-moore.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v3] audit: don't take task_lock() in audit_exe_compare() code path | expand |
On Tue, Oct 24, 2023 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > > The get_task_exe_file() function locks the given task with task_lock() > which when used inside audit_exe_compare() can cause deadlocks on > systems that generate audit records when the task_lock() is held. We > resolve this problem with two changes: ignoring those cases where the > task being audited is not the current task, and changing our approach > to obtaining the executable file struct to not require task_lock(). > > With the intent of the audit exe filter being to filter on audit events > generated by processes started by the specified executable, it makes > sense that we would only want to use the exe filter on audit records > associated with the currently executing process, e.g. @current. If > we are asked to filter records using a non-@current task_struct we can > safely ignore the exe filter without negatively impacting the admin's > expectations for the exe filter. > > Knowing that we only have to worry about filtering the currently > executing task in audit_exe_compare() we can do away with the > task_lock() and call get_mm_exe_file() with @current->mm directly. > > Cc: <stable@vger.kernel.org> > Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") > Reported-by: Andreas Steinmetz <anstein99@googlemail.com> > Reviewed-by: John Johansen <john.johanse@canonical.com> John, I took the liberty of preserving your 'Reviewed-by' tag since I felt the current->mm check was a minor addition, but if you feel otherwise let me know and I'll remove it. > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > - v3 > * added a !current->mm check > - v2 > * dropped mmget()/mmput() > - v1 > * initial revision > --- > kernel/audit_watch.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-)
This does fix the bug, and with the caveat I can't vouch for ignoring non-current in the routine: Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> Thanks for fixing the bug. Pretty weird it took this long for it to surface. On 10/24/23, Paul Moore <paul@paul-moore.com> wrote: > On Tue, Oct 24, 2023 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: >> >> The get_task_exe_file() function locks the given task with task_lock() >> which when used inside audit_exe_compare() can cause deadlocks on >> systems that generate audit records when the task_lock() is held. We >> resolve this problem with two changes: ignoring those cases where the >> task being audited is not the current task, and changing our approach >> to obtaining the executable file struct to not require task_lock(). >> >> With the intent of the audit exe filter being to filter on audit events >> generated by processes started by the specified executable, it makes >> sense that we would only want to use the exe filter on audit records >> associated with the currently executing process, e.g. @current. If >> we are asked to filter records using a non-@current task_struct we can >> safely ignore the exe filter without negatively impacting the admin's >> expectations for the exe filter. >> >> Knowing that we only have to worry about filtering the currently >> executing task in audit_exe_compare() we can do away with the >> task_lock() and call get_mm_exe_file() with @current->mm directly. >> >> Cc: <stable@vger.kernel.org> >> Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") >> Reported-by: Andreas Steinmetz <anstein99@googlemail.com> >> Reviewed-by: John Johansen <john.johanse@canonical.com> > > John, I took the liberty of preserving your 'Reviewed-by' tag since I > felt the current->mm check was a minor addition, but if you feel > otherwise let me know and I'll remove it. > >> Signed-off-by: Paul Moore <paul@paul-moore.com> >> --- >> - v3 >> * added a !current->mm check >> - v2 >> * dropped mmget()/mmput() >> - v1 >> * initial revision >> --- >> kernel/audit_watch.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) > > -- > paul-moore.com >
On Tue, Oct 24, 2023 at 2:48 PM Mateusz Guzik <mjguzik@gmail.com> wrote: > > This does fix the bug, and with the caveat I can't vouch for ignoring > non-current in the routine: Understood, I'm vouching for that bit. > Reviewed-by: Mateusz Guzik <mjguzik@gmail.com> > > Thanks for fixing the bug. Pretty weird it took this long for it to surface. Yeah, I thought the same thing. Regardless, thanks for taking the time to review the fix. > On 10/24/23, Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Oct 24, 2023 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > >> > >> The get_task_exe_file() function locks the given task with task_lock() > >> which when used inside audit_exe_compare() can cause deadlocks on > >> systems that generate audit records when the task_lock() is held. We > >> resolve this problem with two changes: ignoring those cases where the > >> task being audited is not the current task, and changing our approach > >> to obtaining the executable file struct to not require task_lock(). > >> > >> With the intent of the audit exe filter being to filter on audit events > >> generated by processes started by the specified executable, it makes > >> sense that we would only want to use the exe filter on audit records > >> associated with the currently executing process, e.g. @current. If > >> we are asked to filter records using a non-@current task_struct we can > >> safely ignore the exe filter without negatively impacting the admin's > >> expectations for the exe filter. > >> > >> Knowing that we only have to worry about filtering the currently > >> executing task in audit_exe_compare() we can do away with the > >> task_lock() and call get_mm_exe_file() with @current->mm directly. > >> > >> Cc: <stable@vger.kernel.org> > >> Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") > >> Reported-by: Andreas Steinmetz <anstein99@googlemail.com> > >> Reviewed-by: John Johansen <john.johanse@canonical.com> > > > > John, I took the liberty of preserving your 'Reviewed-by' tag since I > > felt the current->mm check was a minor addition, but if you feel > > otherwise let me know and I'll remove it. > > > >> Signed-off-by: Paul Moore <paul@paul-moore.com> > >> --- > >> - v3 > >> * added a !current->mm check > >> - v2 > >> * dropped mmget()/mmput() > >> - v1 > >> * initial revision > >> --- > >> kernel/audit_watch.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > > > > -- > > paul-moore.com > > -- > Mateusz Guzik <mjguzik gmail.com>
On Tue, Oct 24, 2023 at 2:39 PM Paul Moore <paul@paul-moore.com> wrote: > > The get_task_exe_file() function locks the given task with task_lock() > which when used inside audit_exe_compare() can cause deadlocks on > systems that generate audit records when the task_lock() is held. We > resolve this problem with two changes: ignoring those cases where the > task being audited is not the current task, and changing our approach > to obtaining the executable file struct to not require task_lock(). > > With the intent of the audit exe filter being to filter on audit events > generated by processes started by the specified executable, it makes > sense that we would only want to use the exe filter on audit records > associated with the currently executing process, e.g. @current. If > we are asked to filter records using a non-@current task_struct we can > safely ignore the exe filter without negatively impacting the admin's > expectations for the exe filter. > > Knowing that we only have to worry about filtering the currently > executing task in audit_exe_compare() we can do away with the > task_lock() and call get_mm_exe_file() with @current->mm directly. > > Cc: <stable@vger.kernel.org> > Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") > Reported-by: Andreas Steinmetz <anstein99@googlemail.com> > Reviewed-by: John Johansen <john.johanse@canonical.com> > Signed-off-by: Paul Moore <paul@paul-moore.com> > --- > - v3 > * added a !current->mm check > - v2 > * dropped mmget()/mmput() > - v1 > * initial revision > --- > kernel/audit_watch.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) Considering that we are only a few days away from v6.6, I've gone and merged this into audit/dev and will send this up to Linus during the merge window next week.
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c index 65075f1e4ac8..91e82e34b51e 100644 --- a/kernel/audit_watch.c +++ b/kernel/audit_watch.c @@ -527,11 +527,18 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark) unsigned long ino; dev_t dev; - exe_file = get_task_exe_file(tsk); + /* only do exe filtering if we are recording @current events/records */ + if (tsk != current) + return 0; + + if (WARN_ON_ONCE(!current->mm)) + return 0; + exe_file = get_mm_exe_file(current->mm); if (!exe_file) return 0; ino = file_inode(exe_file)->i_ino; dev = file_inode(exe_file)->i_sb->s_dev; fput(exe_file); + return audit_mark_compare(mark, ino, dev); }
The get_task_exe_file() function locks the given task with task_lock() which when used inside audit_exe_compare() can cause deadlocks on systems that generate audit records when the task_lock() is held. We resolve this problem with two changes: ignoring those cases where the task being audited is not the current task, and changing our approach to obtaining the executable file struct to not require task_lock(). With the intent of the audit exe filter being to filter on audit events generated by processes started by the specified executable, it makes sense that we would only want to use the exe filter on audit records associated with the currently executing process, e.g. @current. If we are asked to filter records using a non-@current task_struct we can safely ignore the exe filter without negatively impacting the admin's expectations for the exe filter. Knowing that we only have to worry about filtering the currently executing task in audit_exe_compare() we can do away with the task_lock() and call get_mm_exe_file() with @current->mm directly. Cc: <stable@vger.kernel.org> Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") Reported-by: Andreas Steinmetz <anstein99@googlemail.com> Reviewed-by: John Johansen <john.johanse@canonical.com> Signed-off-by: Paul Moore <paul@paul-moore.com> --- - v3 * added a !current->mm check - v2 * dropped mmget()/mmput() - v1 * initial revision --- kernel/audit_watch.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)