* Re: [PATCH] audit: consistently record PIDs with task_tgid_nr()
From: Jeffrey Vander Stoep @ 2016-08-30 21:58 UTC (permalink / raw)
To: Paul Moore, linux-audit-H+wXaHxf7aLQT0dZR+AlfA; +Cc: LSM List, SELinux
In-Reply-To: <CAGH-KgtOQx6XLStD++n8kBMKNky1t8or7KC2UyYxdaRXjQMhbA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 1279 bytes --]
Can you add tid while you're at it?
We're already looking for it on Android:
https://android-review.googlesource.com/#/c/236952
On Tue, Aug 30, 2016 at 2:15 PM Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, Aug 30, 2016 at 5:13 PM, Paul Moore <pmoore-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > From: Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>
> >
> > Unfortunately we record PIDs in audit records using a variety of
> > methods despite the correct way being the use of task_tgid_nr().
> > This patch converts all of these callers, except for the case of
> > AUDIT_SET in audit_receive_msg() (see the comment in the code).
> >
> > Reported-by: Jeff Vander Stoep <jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Paul Moore <paul-r2n+y4ga6xFZroRs9YW3xA@public.gmane.org>
> > ---
> > kernel/audit.c | 8 +++++++-
> > kernel/auditsc.c | 12 ++++++------
> > security/lsm_audit.c | 4 ++--
> > 3 files changed, 15 insertions(+), 9 deletions(-)
>
> I forgot to tag this with "RFC". This patch compiles but I haven't
> had a chance to test it yet so it isn't going into audit#next just
> yet; if you have any concerns, now is the time to voice them.
>
> --
> paul moore
> security @ redhat
>
[-- Attachment #1.2: Type: text/html, Size: 2181 bytes --]
[-- Attachment #2: Type: text/plain, Size: 304 bytes --]
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply
* Re: [PATCH] audit: consistently record PIDs with task_tgid_nr()
From: Paul Moore @ 2016-08-30 21:15 UTC (permalink / raw)
To: Jeff Vander Stoep, linux-audit; +Cc: linux-security-module, selinux
In-Reply-To: <147259160184.15526.16504125805093739705.stgit@localhost>
On Tue, Aug 30, 2016 at 5:13 PM, Paul Moore <pmoore@redhat.com> wrote:
> From: Paul Moore <paul@paul-moore.com>
>
> Unfortunately we record PIDs in audit records using a variety of
> methods despite the correct way being the use of task_tgid_nr().
> This patch converts all of these callers, except for the case of
> AUDIT_SET in audit_receive_msg() (see the comment in the code).
>
> Reported-by: Jeff Vander Stoep <jeffv@google.com>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
> kernel/audit.c | 8 +++++++-
> kernel/auditsc.c | 12 ++++++------
> security/lsm_audit.c | 4 ++--
> 3 files changed, 15 insertions(+), 9 deletions(-)
I forgot to tag this with "RFC". This patch compiles but I haven't
had a chance to test it yet so it isn't going into audit#next just
yet; if you have any concerns, now is the time to voice them.
--
paul moore
security @ redhat
^ permalink raw reply
* [PATCH] audit: consistently record PIDs with task_tgid_nr()
From: Paul Moore @ 2016-08-30 21:13 UTC (permalink / raw)
To: Jeff Vander Stoep, linux-audit; +Cc: linux-security-module, selinux
From: Paul Moore <paul@paul-moore.com>
Unfortunately we record PIDs in audit records using a variety of
methods despite the correct way being the use of task_tgid_nr().
This patch converts all of these callers, except for the case of
AUDIT_SET in audit_receive_msg() (see the comment in the code).
Reported-by: Jeff Vander Stoep <jeffv@google.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
---
kernel/audit.c | 8 +++++++-
kernel/auditsc.c | 12 ++++++------
security/lsm_audit.c | 4 ++--
3 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 8d528f9..02bde12 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -877,6 +877,12 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
return err;
}
if (s.mask & AUDIT_STATUS_PID) {
+ /* NOTE: we are using task_tgid_vnr() below because
+ * the s.pid value is relative to the namespace
+ * of the caller; at present this doesn't matter
+ * much since you can really only run auditd
+ * from the initial pid namespace, but something
+ * to keep in mind if this changes */
int new_pid = s.pid;
pid_t requesting_pid = task_tgid_vnr(current);
@@ -1917,7 +1923,7 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
" euid=%u suid=%u fsuid=%u"
" egid=%u sgid=%u fsgid=%u tty=%s ses=%u",
task_ppid_nr(tsk),
- task_pid_nr(tsk),
+ task_tgid_nr(tsk),
from_kuid(&init_user_ns, audit_get_loginuid(tsk)),
from_kuid(&init_user_ns, cred->uid),
from_kgid(&init_user_ns, cred->gid),
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 2672d10..3824b1b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -455,7 +455,7 @@ static int audit_filter_rules(struct task_struct *tsk,
switch (f->type) {
case AUDIT_PID:
- pid = task_pid_nr(tsk);
+ pid = task_tgid_nr(tsk);
result = audit_comparator(pid, f->op, f->val);
break;
case AUDIT_PPID:
@@ -1993,7 +1993,7 @@ static void audit_log_set_loginuid(kuid_t koldloginuid, kuid_t kloginuid,
loginuid = from_kuid(&init_user_ns, kloginuid),
tty = audit_get_tty(current);
- audit_log_format(ab, "pid=%d uid=%u", task_pid_nr(current), uid);
+ audit_log_format(ab, "pid=%d uid=%u", task_tgid_nr(current), uid);
audit_log_task_context(ab);
audit_log_format(ab, " old-auid=%u auid=%u tty=%s old-ses=%u ses=%u res=%d",
oldloginuid, loginuid, tty ? tty_name(tty) : "(none)",
@@ -2220,7 +2220,7 @@ void __audit_ptrace(struct task_struct *t)
{
struct audit_context *context = current->audit_context;
- context->target_pid = task_pid_nr(t);
+ context->target_pid = task_tgid_nr(t);
context->target_auid = audit_get_loginuid(t);
context->target_uid = task_uid(t);
context->target_sessionid = audit_get_sessionid(t);
@@ -2245,7 +2245,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
if (audit_pid && t->tgid == audit_pid) {
if (sig == SIGTERM || sig == SIGHUP || sig == SIGUSR1 || sig == SIGUSR2) {
- audit_sig_pid = task_pid_nr(tsk);
+ audit_sig_pid = task_tgid_nr(tsk);
if (uid_valid(tsk->loginuid))
audit_sig_uid = tsk->loginuid;
else
@@ -2345,7 +2345,7 @@ int __audit_log_bprm_fcaps(struct linux_binprm *bprm,
void __audit_log_capset(const struct cred *new, const struct cred *old)
{
struct audit_context *context = current->audit_context;
- context->capset.pid = task_pid_nr(current);
+ context->capset.pid = task_tgid_nr(current);
context->capset.cap.effective = new->cap_effective;
context->capset.cap.inheritable = new->cap_effective;
context->capset.cap.permitted = new->cap_permitted;
@@ -2377,7 +2377,7 @@ static void audit_log_task(struct audit_buffer *ab)
from_kgid(&init_user_ns, gid),
sessionid);
audit_log_task_context(ab);
- audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
+ audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
audit_log_untrustedstring(ab, get_task_comm(comm, current));
audit_log_d_path_exe(ab, current->mm);
}
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index cccbf30..45d927a 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -220,7 +220,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
*/
BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
- audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
+ audit_log_format(ab, " pid=%d comm=", task_tgid_nr(current));
audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
switch (a->type) {
@@ -294,7 +294,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
case LSM_AUDIT_DATA_TASK: {
struct task_struct *tsk = a->u.tsk;
if (tsk) {
- pid_t pid = task_pid_nr(tsk);
+ pid_t pid = task_tgid_nr(tsk);
if (pid) {
char comm[sizeof(tsk->comm)];
audit_log_format(ab, " opid=%d ocomm=", pid);
^ permalink raw reply related
* Re: [PATCH] security: lsm_audit: print pid and tid
From: Paul Moore @ 2016-08-30 20:56 UTC (permalink / raw)
To: Jeff Vander Stoep
Cc: linux-audit-H+wXaHxf7aLQT0dZR+AlfA,
selinux-+05T5uksL2qpZYMLLGbcSA
In-Reply-To: <1469544870-11574-1-git-send-email-jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Tue, Jul 26, 2016 at 10:54 AM, Jeff Vander Stoep <jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> dump_common_audit_data() currently contains a field for pid, but the
> value printed is actually the thread ID, tid. Update this value to
> return the task group ID. Add a new field for tid. With this change
> the values printed by audit now match the values returned by the
> getpid() and gettid() syscalls.
>
> Signed-off-by: Jeff Vander Stoep <jeffv-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
> security/lsm_audit.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Unfortunately while reviewing this code I realized that the problem
extends far beyond just lsm_audit.c, kernel/audit*.c is affected as
well. It's actually much worse in kernel/audit*.c as it is very
inconsistent.
I was going to ask you to fix the other code too while you were at it,
but I realized what it was easy enough to fix it while I was going
through the code anyway ... expect a patch shortly.
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index cccbf30..57f26c1 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -220,7 +220,8 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> */
> BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>
> - audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> + audit_log_format(ab, " pid=%d tid=%d comm=", task_tgid_vnr(tsk),
> + task_pid_vnr(tsk));
> audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
>
> switch (a->type) {
> @@ -294,10 +295,12 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> case LSM_AUDIT_DATA_TASK: {
> struct task_struct *tsk = a->u.tsk;
> if (tsk) {
> - pid_t pid = task_pid_nr(tsk);
> + pid_t pid = task_tgid_vnr(tsk);
> if (pid) {
> char comm[sizeof(tsk->comm)];
> audit_log_format(ab, " opid=%d ocomm=", pid);
> + audit_log_format(ab, " opid=%d otid=%d ocomm=",
> + pid, task_pid_vnr(tsk));
> audit_log_untrustedstring(ab,
> memcpy(comm, tsk->comm, sizeof(comm)));
> }
> --
> 2.8.0.rc3.226.g39d4020
--
paul moore
www.paul-moore.com
_______________________________________________
Selinux mailing list
Selinux-+05T5uksL2qpZYMLLGbcSA@public.gmane.org
To unsubscribe, send email to Selinux-leave-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
To get help, send an email containing "help" to Selinux-request-+05T5uksL2pAGbPMOrvdOA@public.gmane.org
^ permalink raw reply
* Re: [PATCH] security: lsm_audit: print pid and tid
From: Paul Moore @ 2016-08-30 20:18 UTC (permalink / raw)
To: Steve Grubb; +Cc: Jeff Vander Stoep, linux-audit, selinux
In-Reply-To: <1761452.xxMEsoZRlS@x2>
On Tue, Aug 30, 2016 at 11:03 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, August 17, 2016 4:58:02 PM EDT Paul Moore wrote:
>> On Tue, Jul 26, 2016 at 10:54 AM, Jeff Vander Stoep <jeffv@google.com> wrote:
>> > dump_common_audit_data() currently contains a field for pid, but the
>> > value printed is actually the thread ID, tid. Update this value to
>> > return the task group ID. Add a new field for tid. With this change
>> > the values printed by audit now match the values returned by the
>> > getpid() and gettid() syscalls.
>> >
>> > Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
>> > ---
>> >
>> > security/lsm_audit.c | 7 +++++--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> Hi Jeff,
>>
>> Have you tested this against the audit-testsuite[1]? We don't have an
>> explicit PID test yet, but at least two of the tests do test it as a
>> side effect.
>>
>> Steve, I don't see the thread ID listed in the field dictionary, are
>> you okay with using "tid" for this?
>
> Yes. Can someone add both?
Yes, I'll add "tid" to the field DB once we commit the kernel patch.
>> However, as far as I can see, the biggest problem with this patch is
>> that it adds a field in the middle of a record which will likely cause
>> the audit userspace tools to explode (or so I've been warned in the
>> past). Steve, what say you about the userspace?
>
> This is OK. After picking out pid, search utiliies scan for comm. They will
> just skip over the new field. If fields that we normally search change order,
> then we have a problem.
>
> So, ACK on my end.
Okay, thanks. If we blow up your userspace I'll remind you of this
conversation ;)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Mateusz Guzik @ 2016-08-30 20:13 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Konstantin Khlebnikov, ebiederm, oleg, sgrubb, pmoore, eparis,
luto, linux-audit, linux-kernel, Al Viro
In-Reply-To: <20160830185021.GL5983@madcap2.tricolour.ca>
On Tue, Aug 30, 2016 at 02:50:21PM -0400, Richard Guy Briggs wrote:
> On 2016-08-23 16:20, Mateusz Guzik wrote:
> > audit_exe_compare directly accesses mm->exe_file without making sure the
> > object is stable. Fixing it using current primitives results in
> > partially duplicating what proc_exe_link is doing.
> >
> > As such, introduce a trivial helper which can be used in both places and
> > fix the func.
> >
> > Changes since v1:
> > * removed an unused 'out' label which crept in
> >
> > Mateusz Guzik (2):
> > mm: introduce get_task_exe_file
> > audit: fix exe_file access in audit_exe_compare
>
> The task_lock affects a much bigger struct than the mm ref count. Is
> this really necessary? Is a spin-lock significantly lower cost than a
> refcount? Other than that, this refactorization looks sensible.
>
proc_exe_link was taking the lock anyway to guarantee a stable mm.
I think the helper cleans the code up a little bit and there is
microoptimisation to not play with the refcount.
If audit_exe_compare has guarantees the task wont reach exit_mm, it can
use get_mm_exe_file which means the atomic op would be only on the file
object.
I was under the impression this is the expected behaviour, but your
patch used the task lock to grab mm, so I mimicked it here.
--
Mateusz Guzik
^ permalink raw reply
* Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Richard Guy Briggs @ 2016-08-30 18:50 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Konstantin Khlebnikov, ebiederm, oleg, sgrubb, pmoore, eparis,
luto, linux-audit, linux-kernel, Al Viro
In-Reply-To: <1471962039-14940-1-git-send-email-mguzik@redhat.com>
On 2016-08-23 16:20, Mateusz Guzik wrote:
> audit_exe_compare directly accesses mm->exe_file without making sure the
> object is stable. Fixing it using current primitives results in
> partially duplicating what proc_exe_link is doing.
>
> As such, introduce a trivial helper which can be used in both places and
> fix the func.
>
> Changes since v1:
> * removed an unused 'out' label which crept in
>
> Mateusz Guzik (2):
> mm: introduce get_task_exe_file
> audit: fix exe_file access in audit_exe_compare
The task_lock affects a much bigger struct than the mm ref count. Is
this really necessary? Is a spin-lock significantly lower cost than a
refcount? Other than that, this refactorization looks sensible.
Acked-by: Richard Guy Briggs <rgb@redhat.com>
> fs/proc/base.c | 7 +------
> include/linux/mm.h | 1 +
> kernel/audit_watch.c | 8 +++++---
> kernel/fork.c | 23 +++++++++++++++++++++++
> 4 files changed, 30 insertions(+), 9 deletions(-)
>
> --
> 1.8.3.1
>
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH] security: lsm_audit: print pid and tid
From: Steve Grubb @ 2016-08-30 15:03 UTC (permalink / raw)
To: Paul Moore; +Cc: Jeff Vander Stoep, linux-audit, selinux
In-Reply-To: <CAHC9VhS=odoi8NFFGP36VAMcL_Gbbin+0pyTj-MNcsPZKit0GQ@mail.gmail.com>
On Wednesday, August 17, 2016 4:58:02 PM EDT Paul Moore wrote:
> On Tue, Jul 26, 2016 at 10:54 AM, Jeff Vander Stoep <jeffv@google.com> wrote:
> > dump_common_audit_data() currently contains a field for pid, but the
> > value printed is actually the thread ID, tid. Update this value to
> > return the task group ID. Add a new field for tid. With this change
> > the values printed by audit now match the values returned by the
> > getpid() and gettid() syscalls.
> >
> > Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
> > ---
> >
> > security/lsm_audit.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
>
> Hi Jeff,
>
> Have you tested this against the audit-testsuite[1]? We don't have an
> explicit PID test yet, but at least two of the tests do test it as a
> side effect.
>
> Steve, I don't see the thread ID listed in the field dictionary, are
> you okay with using "tid" for this?
Yes. Can someone add both?
> However, as far as I can see, the biggest problem with this patch is
> that it adds a field in the middle of a record which will likely cause
> the audit userspace tools to explode (or so I've been warned in the
> past). Steve, what say you about the userspace?
This is OK. After picking out pid, search utiliies scan for comm. They will
just skip over the new field. If fields that we normally search change order,
then we have a problem.
So, ACK on my end.
-Steve
> [1] https://github.com/linux-audit/audit-testsuite
> [2]
> https://github.com/linux-audit/audit-documentation/blob/master/specs/fields
> /field-dictionary.csv
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index cccbf30..57f26c1 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -220,7 +220,8 @@ static void dump_common_audit_data(struct audit_buffer
> > *ab,>
> > */
> >
> > BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> >
> > - audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> > + audit_log_format(ab, " pid=%d tid=%d comm=", task_tgid_vnr(tsk),
> > + task_pid_vnr(tsk));
> >
> > audit_log_untrustedstring(ab, memcpy(comm, current->comm,
> > sizeof(comm)));
> >
> > switch (a->type) {
> >
> > @@ -294,10 +295,12 @@ static void dump_common_audit_data(struct
> > audit_buffer *ab,>
> > case LSM_AUDIT_DATA_TASK: {
> >
> > struct task_struct *tsk = a->u.tsk;
> > if (tsk) {
> >
> > - pid_t pid = task_pid_nr(tsk);
> > + pid_t pid = task_tgid_vnr(tsk);
> >
> > if (pid) {
> >
> > char comm[sizeof(tsk->comm)];
> > audit_log_format(ab, " opid=%d ocomm=",
> > pid);
> >
> > + audit_log_format(ab, " opid=%d otid=%d
> > ocomm=", + pid,
> > task_pid_vnr(tsk));>
> > audit_log_untrustedstring(ab,
> >
> > memcpy(comm, tsk->comm,
> > sizeof(comm)));
> >
> > }
^ permalink raw reply
* Re: [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Paul Moore @ 2016-08-29 22:50 UTC (permalink / raw)
To: Mateusz Guzik, linux-kernel
Cc: Konstantin Khlebnikov, Richard Guy Briggs, oleg, luto,
linux-audit, ebiederm, Al Viro
In-Reply-To: <1471962039-14940-1-git-send-email-mguzik@redhat.com>
On Tue, Aug 23, 2016 at 10:20 AM, Mateusz Guzik <mguzik@redhat.com> wrote:
> audit_exe_compare directly accesses mm->exe_file without making sure the
> object is stable. Fixing it using current primitives results in
> partially duplicating what proc_exe_link is doing.
>
> As such, introduce a trivial helper which can be used in both places and
> fix the func.
>
> Changes since v1:
> * removed an unused 'out' label which crept in
>
> Mateusz Guzik (2):
> mm: introduce get_task_exe_file
> audit: fix exe_file access in audit_exe_compare
>
> fs/proc/base.c | 7 +------
> include/linux/mm.h | 1 +
> kernel/audit_watch.c | 8 +++++---
> kernel/fork.c | 23 +++++++++++++++++++++++
> 4 files changed, 30 insertions(+), 9 deletions(-)
Thanks for doing this.
Both patches look fine to me, does anyone in the mm area have any
objections? If not, I'll merge these into the audit tree and mark
them for stable.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH] security: lsm_audit: print pid and tid
From: Paul Moore @ 2016-08-29 22:42 UTC (permalink / raw)
To: sgrubb; +Cc: Jeff Vander Stoep, linux-audit, selinux
In-Reply-To: <CAHC9VhS=odoi8NFFGP36VAMcL_Gbbin+0pyTj-MNcsPZKit0GQ@mail.gmail.com>
On Wed, Aug 17, 2016 at 4:58 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 26, 2016 at 10:54 AM, Jeff Vander Stoep <jeffv@google.com> wrote:
>> dump_common_audit_data() currently contains a field for pid, but the
>> value printed is actually the thread ID, tid. Update this value to
>> return the task group ID. Add a new field for tid. With this change
>> the values printed by audit now match the values returned by the
>> getpid() and gettid() syscalls.
>>
>> Signed-off-by: Jeff Vander Stoep <jeffv@google.com>
>> ---
>> security/lsm_audit.c | 7 +++++--
>> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> Hi Jeff,
>
> Have you tested this against the audit-testsuite[1]? We don't have an
> explicit PID test yet, but at least two of the tests do test it as a
> side effect.
>
> Steve, I don't see the thread ID listed in the field dictionary, are
> you okay with using "tid" for this?
>
> However, as far as I can see, the biggest problem with this patch is
> that it adds a field in the middle of a record which will likely cause
> the audit userspace tools to explode (or so I've been warned in the
> past). Steve, what say you about the userspace?
>
> [1] https://github.com/linux-audit/audit-testsuite
> [2] https://github.com/linux-audit/audit-documentation/blob/master/specs/fields/field-dictionary.csv
Steve?
>> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
>> index cccbf30..57f26c1 100644
>> --- a/security/lsm_audit.c
>> +++ b/security/lsm_audit.c
>> @@ -220,7 +220,8 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> */
>> BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
>>
>> - audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
>> + audit_log_format(ab, " pid=%d tid=%d comm=", task_tgid_vnr(tsk),
>> + task_pid_vnr(tsk));
>> audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
>>
>> switch (a->type) {
>> @@ -294,10 +295,12 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>> case LSM_AUDIT_DATA_TASK: {
>> struct task_struct *tsk = a->u.tsk;
>> if (tsk) {
>> - pid_t pid = task_pid_nr(tsk);
>> + pid_t pid = task_tgid_vnr(tsk);
>> if (pid) {
>> char comm[sizeof(tsk->comm)];
>> audit_log_format(ab, " opid=%d ocomm=", pid);
>> + audit_log_format(ab, " opid=%d otid=%d ocomm=",
>> + pid, task_pid_vnr(tsk));
>> audit_log_untrustedstring(ab,
>> memcpy(comm, tsk->comm, sizeof(comm)));
>> }
>
> --
> paul moore
> www.paul-moore.com
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: RHEL-7 and implementing audit rules
From: Steve Grubb @ 2016-08-23 17:53 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <CAJdJdQk04ctomn=KM1nhH6dm88yjHbnUXEz_c_h3fbkRineZHw@mail.gmail.com>
On Tuesday, August 23, 2016 1:32:48 PM EDT warron.french wrote:
> In RHEL-6, audit rules were added directly to */etc/audit/audit.rules*, but
> it seems that it is a requirement in RHEL-7 to be placed directly in a file
> (any file?) within
>
> */etc/audit/rules.d/.*
Well, to be honest, you can do that on RHEL6, too. And on RHEL7 you can go
back to the old method. Just copy
/lib/systemd/system/auditd.service to /etc/systemd/system/ and edit the file to
comment out augenrules and uncomment auditctl. On RHEL7 the default config is
changed so that its more "enterprisey". There is also a README-rules file that
gives some tips on using this new rules.d directory.
> I discovered this by doing some man-page reading of the audit.rules file
> after my RHEL-6-variant understanding was turned on its ear. So, I created
> an */etc/audit/rules.d/audit.rules* and added my rules in there.
>
> I ensured that I set "-e 1" because the value wasn't already set. I added
> a watch rules (-w) and it at first didn't take effect; so then realized,
> "*this is RHEL-7, I have to use **systemctl* to restart services."
Actually, auditd is the one thing that cannot use systemd because of dbus
activation. So, the service command is still what you have to use.
> That also didn't work. I tested with auditctl -l and looked for my new
> rules (only 2 of them); so a reboot was committed for something else by a
> coworker, and then the *auditctl -l* command actually did display updated
> rules. This is very confusing, but I thought nothing more about it,
> figuring it is a flaw somewhere.
>
> Anyway, today I added an action rule (-a/Syscall Rule) and it too has not
> taken effect; not after a *service auditd restart*, not after a *systemctl
> restart auditd.service*, just nothing. I also recently read in a community
> post, today, that systemctl doesn't handle the restart of auditd very well
> (the comment came from you Mr. Grubb).
>
> I cannot reboot the server yet, and quite frankly I don't want to be forced
> to reboot the server everytime I add a rule - it's a lab, not production.
Run augenrules --load, you can test prior with augenrules --check
> Can someone please tell me what I am doing so wrong, with respect to
> handling audit configurations on a RHEL-7 system, and tell me how to work
> the processes correctly?
I don't know if there is a problem with systemd not honoring the ExecStartPost
action on a restart, but that kind of sounds like what's happening.
-Steve
^ permalink raw reply
* RHEL-7 and implementing audit rules
From: warron.french @ 2016-08-23 17:32 UTC (permalink / raw)
To: linux-audit
[-- Attachment #1.1: Type: text/plain, Size: 1885 bytes --]
Hi, I am back again.
I have some experience and a great deal more comfort with the Linux Audit
configurations nowadays. I learned an aweful lot by working with CentOS-6;
however, this question is focused purely on RHEL-7.
In RHEL-6, audit rules were added directly to */etc/audit/audit.rules*, but
it seems that it is a requirement in RHEL-7 to be placed directly in a file
(any file?) within
*/etc/audit/rules.d/.*
I discovered this by doing some man-page reading of the audit.rules file
after my RHEL-6-variant understanding was turned on its ear. So, I created
an */etc/audit/rules.d/audit.rules* and added my rules in there.
I ensured that I set "-e 1" because the value wasn't already set. I added
a watch rules (-w) and it at first didn't take effect; so then realized, "*this
is RHEL-7, I have to use **systemctl* to restart services."
That also didn't work. I tested with auditctl -l and looked for my new
rules (only 2 of them); so a reboot was committed for something else by a
coworker, and then the *auditctl -l* command actually did display updated
rules. This is very confusing, but I thought nothing more about it,
figuring it is a flaw somewhere.
Anyway, today I added an action rule (-a/Syscall Rule) and it too has not
taken effect; not after a *service auditd restart*, not after a *systemctl
restart auditd.service*, just nothing. I also recently read in a community
post, today, that systemctl doesn't handle the restart of auditd very well
(the comment came from you Mr. Grubb).
I cannot reboot the server yet, and quite frankly I don't want to be forced
to reboot the server everytime I add a rule - it's a lab, not production.
Can someone please tell me what I am doing so wrong, with respect to
handling audit configurations on a RHEL-7 system, and tell me how to work
the processes correctly?
Thanks,
--------------------------
Warron French
[-- Attachment #1.2: Type: text/html, Size: 2792 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCHv2 1/2] mm: introduce get_task_exe_file
From: Mateusz Guzik @ 2016-08-23 14:52 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Konstantin Khlebnikov, linux-kernel, luto, Richard Guy Briggs,
linux-audit, ebiederm, Al Viro
In-Reply-To: <20160823144812.GA2088@redhat.com>
On Tue, Aug 23, 2016 at 04:48:13PM +0200, Oleg Nesterov wrote:
> On 08/23, Mateusz Guzik wrote:
> >
> > +struct file *get_task_exe_file(struct task_struct *task)
> > +{
> > + struct file *exe_file = NULL;
> > + struct mm_struct *mm;
> > +
> > + task_lock(task);
> > + mm = task->mm;
> > + if (mm) {
> > + if (!(task->flags & PF_KTHREAD))
> > + exe_file = get_mm_exe_file(mm);
> > + }
> > + task_unlock(task);
> > + return exe_file;
> > +}
>
> I can't believe I am going to comment the coding style but I can't resist ;)
>
> if (mm && !(task->flags & PF_KTHREAD)))
> exe_file = get_mm_exe_file(mm);
>
> looks a bit simpler to me. But this is purely cosmetic and subjective,
> both patches look good to me.
>
Actually I did it for some consistency with get_task_mm.
The check can likely be done prior to taking the lock in both functions
and that would clean them up a little bit, but I wanted to avoid nit
picking... :>
> Acked-by: Oleg Nesterov <oleg@redhat.com>
>
Thanks
--
Mateusz Guzik
^ permalink raw reply
* Re: [GIT PULL] [PATCH v4 00/26] Delete CURRENT_TIME and CURRENT_TIME_SEC macros
From: Arnd Bergmann @ 2016-08-23 14:51 UTC (permalink / raw)
To: Greg KH
Cc: shaggy-DgEjT+Ai2ygdnm+yROfE0A,
jfs-discussion-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
trond.myklebust-7I+n7zu2hftEKMMhf/gKZA,
adrian.hunter-ral2JQCrhuEAvxtiuMwx3w, clm-b10kYP2dOMg,
adilger.kernel-m1MBpc4rdrD3fQ9qLvQP4Q, Deepa Dinamani,
buchino-FYB4Gu1CFyUAvxtiuMwx3w, tglx-hfZtesqFncYOwBW4kG4KsQ,
zyan-H+wXaHxf7aLQT0dZR+AlfA,
jejb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
paul-r2n+y4ga6xFZroRs9YW3xA, linux-scsi-u79uwXL29TY76Z2rM5mHXA,
y2038-cunTk1MwBs8s++Sfvej+rw, idryomov-Re5JQEeQqe8AvxtiuMwx3w,
linux-ext4-u79uwXL29TY76Z2rM5mHXA,
cm224.lee-Sze3O3UU22JBDgjK7y7TUQ, dushistov-JGs/UdohzUI,
mfasheh-IBi9RG/b67k, sramars-FYB4Gu1CFyUAvxtiuMwx3w,
john.stultz-QSEj5FYQhm4dnm+yROfE0A,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, dsterba-IBi9RG/b67k,
jaegeuk-DgEjT+Ai2ygdnm+yROfE0A, ceph-devel-u79uwXL29TY76Z2rM5mHXA,
linux-nfs-u79uwXL29TY76Z2rM5mHXA, elder-DgEjT+Ai2ygdnm+yROfE0A,
tytso-3s7WtUTddSA, sage-H+wXaHxf7aLQT0dZR+AlfA,
martin.petersen-QHcLZuEGTsvQT0dZR+AlfA,
dedekind1-Re5JQEeQqe8AvxtiuMwx3w, jbacik-b10kYP2dOMg,
hiralpat-FYB4Gu1CFyUAvxtiuMwx3w,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
eparis-H+wXaHxf7aLQT0dZR+AlfA,
linux-f2fs-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
sfrench-eUNUBHrolfbYtjvyW6yDsg,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA
In-Reply-To: <20160815162312.GA19794-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Monday, August 15, 2016 6:23:12 PM CEST Greg KH wrote:
> On Sat, Aug 13, 2016 at 03:48:12PM -0700, Deepa Dinamani wrote:
> > The series is aimed at getting rid of CURRENT_TIME and CURRENT_TIME_SEC macros.
> > The macros are not y2038 safe. There is no plan to transition them into being
> > y2038 safe.
> > ktime_get_* api's can be used in their place. And, these are y2038 safe.
>
> Who are you execting to pull this huge patch series?
Dave Chinner suggested to have Al Viro pick up the whole series.
> Why not just introduce the new api call, wait for that to be merged, and
> then push the individual patches through the different subsystems?
> After half of those get ignored, then provide a single set of patches
> that can go through Andrew or my trees.
That was the original approach for v4.7, but (along with requesting
a number of reworks that Deepa incorporated), Linus preferred doing
the API change done in one chunk, see
https://patchwork.kernel.org/patch/9134249/
Arnd
^ permalink raw reply
* Re: [PATCHv2 1/2] mm: introduce get_task_exe_file
From: Oleg Nesterov @ 2016-08-23 14:48 UTC (permalink / raw)
To: Mateusz Guzik
Cc: Konstantin Khlebnikov, Richard Guy Briggs, ebiederm, sgrubb,
pmoore, eparis, luto, linux-audit, linux-kernel, Al Viro
In-Reply-To: <1471962039-14940-2-git-send-email-mguzik@redhat.com>
On 08/23, Mateusz Guzik wrote:
>
> +struct file *get_task_exe_file(struct task_struct *task)
> +{
> + struct file *exe_file = NULL;
> + struct mm_struct *mm;
> +
> + task_lock(task);
> + mm = task->mm;
> + if (mm) {
> + if (!(task->flags & PF_KTHREAD))
> + exe_file = get_mm_exe_file(mm);
> + }
> + task_unlock(task);
> + return exe_file;
> +}
I can't believe I am going to comment the coding style but I can't resist ;)
if (mm && !(task->flags & PF_KTHREAD)))
exe_file = get_mm_exe_file(mm);
looks a bit simpler to me. But this is purely cosmetic and subjective,
both patches look good to me.
Acked-by: Oleg Nesterov <oleg@redhat.com>
^ permalink raw reply
* [PATCHv2 2/2] audit: fix exe_file access in audit_exe_compare
From: Mateusz Guzik @ 2016-08-23 14:20 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
In-Reply-To: <1471962039-14940-1-git-send-email-mguzik@redhat.com>
Prior to the change the function would blindly deference mm, exe_file
and exe_file->f_inode, each of which could have been NULL or freed.
Use get_task_exe_file to safely obtain stable exe_file.
Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
kernel/audit_watch.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index d6709eb..0d302a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/file.h>
#include <linux/kernel.h>
#include <linux/audit.h>
#include <linux/kthread.h>
@@ -544,10 +545,11 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
unsigned long ino;
dev_t dev;
- rcu_read_lock();
- exe_file = rcu_dereference(tsk->mm->exe_file);
+ exe_file = get_task_exe_file(tsk);
+ if (!exe_file)
+ return 0;
ino = exe_file->f_inode->i_ino;
dev = exe_file->f_inode->i_sb->s_dev;
- rcu_read_unlock();
+ fput(exe_file);
return audit_mark_compare(mark, ino, dev);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCHv2 1/2] mm: introduce get_task_exe_file
From: Mateusz Guzik @ 2016-08-23 14:20 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
In-Reply-To: <1471962039-14940-1-git-send-email-mguzik@redhat.com>
For more convenient access if one has a pointer to the task.
As a minor nit take advantage of the fact that only task lock + rcu are
needed to safely grab ->exe_file. This saves mm refcount dance.
Use the helper in proc_exe_link.
Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
---
fs/proc/base.c | 7 +------
include/linux/mm.h | 1 +
kernel/fork.c | 23 +++++++++++++++++++++++
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2ed41cb..ebccdc1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1556,18 +1556,13 @@ static const struct file_operations proc_pid_set_comm_operations = {
static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
{
struct task_struct *task;
- struct mm_struct *mm;
struct file *exe_file;
task = get_proc_task(d_inode(dentry));
if (!task)
return -ENOENT;
- mm = get_task_mm(task);
+ exe_file = get_task_exe_file(task);
put_task_struct(task);
- if (!mm)
- return -ENOENT;
- exe_file = get_mm_exe_file(mm);
- mmput(mm);
if (exe_file) {
*exe_path = exe_file->f_path;
path_get(&exe_file->f_path);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9d85402..f4e639e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2014,6 +2014,7 @@ extern void mm_drop_all_locks(struct mm_struct *mm);
extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
extern struct file *get_mm_exe_file(struct mm_struct *mm);
+extern struct file *get_task_exe_file(struct task_struct *task);
extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages);
extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages);
diff --git a/kernel/fork.c b/kernel/fork.c
index 6fe775c..a4b2384 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -800,6 +800,29 @@ struct file *get_mm_exe_file(struct mm_struct *mm)
EXPORT_SYMBOL(get_mm_exe_file);
/**
+ * get_task_exe_file - acquire a reference to the task's executable file
+ *
+ * Returns %NULL if task's mm (if any) has no associated executable file or
+ * this is a kernel thread with borrowed mm (see the comment above get_task_mm).
+ * User must release file via fput().
+ */
+struct file *get_task_exe_file(struct task_struct *task)
+{
+ struct file *exe_file = NULL;
+ struct mm_struct *mm;
+
+ task_lock(task);
+ mm = task->mm;
+ if (mm) {
+ if (!(task->flags & PF_KTHREAD))
+ exe_file = get_mm_exe_file(mm);
+ }
+ task_unlock(task);
+ return exe_file;
+}
+EXPORT_SYMBOL(get_task_exe_file);
+
+/**
* get_task_mm - acquire a reference to the task's mm
*
* Returns %NULL if the task has no mm. Checks PF_KTHREAD (meaning
--
1.8.3.1
^ permalink raw reply related
* [PATCHv2 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Mateusz Guzik @ 2016-08-23 14:20 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
audit_exe_compare directly accesses mm->exe_file without making sure the
object is stable. Fixing it using current primitives results in
partially duplicating what proc_exe_link is doing.
As such, introduce a trivial helper which can be used in both places and
fix the func.
Changes since v1:
* removed an unused 'out' label which crept in
Mateusz Guzik (2):
mm: introduce get_task_exe_file
audit: fix exe_file access in audit_exe_compare
fs/proc/base.c | 7 +------
include/linux/mm.h | 1 +
kernel/audit_watch.c | 8 +++++---
kernel/fork.c | 23 +++++++++++++++++++++++
4 files changed, 30 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Konstantin Khlebnikov @ 2016-08-23 8:34 UTC (permalink / raw)
To: Mateusz Guzik, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
In-Reply-To: <1471899083-7937-1-git-send-email-mguzik@redhat.com>
On 22.08.2016 23:51, Mateusz Guzik wrote:
> audit_exe_compare directly accesses mm->exe_file without making sure the
> object is stable. Fixing it using current primitives results in
> partially duplicating what proc_exe_link is doing.
>
> As such, introduce a trivial helper which can be used in both places and
> fix the func.
Looks good. Except trivial warning that test bot found.
Acked-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
>
> Mateusz Guzik (2):
> mm: introduce get_task_exe_file
> audit: fix exe_file access in audit_exe_compare
>
> fs/proc/base.c | 7 +------
> include/linux/mm.h | 1 +
> kernel/audit_watch.c | 8 +++++---
> kernel/fork.c | 24 ++++++++++++++++++++++++
> 4 files changed, 31 insertions(+), 9 deletions(-)
>
--
Konstantin
^ permalink raw reply
* Re: [PATCH 1/2] mm: introduce get_task_exe_file
From: kbuild test robot @ 2016-08-22 21:49 UTC (permalink / raw)
To: Mateusz Guzik
Cc: kbuild-all, Konstantin Khlebnikov, Richard Guy Briggs, ebiederm,
oleg, sgrubb, pmoore, eparis, luto, linux-audit, linux-kernel,
Al Viro
In-Reply-To: <1471899083-7937-2-git-send-email-mguzik@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
Hi Mateusz,
[auto build test WARNING on linus/master]
[also build test WARNING on v4.8-rc3 next-20160822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Mateusz-Guzik/mm-introduce-get_task_exe_file/20160823-045421
config: sparc64-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
kernel/fork.c: In function 'get_task_exe_file':
>> kernel/fork.c:820:1: warning: label 'out' defined but not used [-Wunused-label]
out:
^
vim +/out +820 kernel/fork.c
804 * Returns %NULL if task's mm (if any) has no associated executable file or
805 * this is a kernel thread with borrowed mm (see the comment above get_task_mm).
806 * User must release file via fput().
807 */
808 struct file *get_task_exe_file(struct task_struct *task)
809 {
810 struct file *exe_file = NULL;
811 struct mm_struct *mm;
812
813 task_lock(task);
814 mm = task->mm;
815 if (mm) {
816 if (!(task->flags & PF_KTHREAD))
817 exe_file = get_mm_exe_file(mm);
818 }
819 task_unlock(task);
> 820 out:
821 return exe_file;
822 }
823 EXPORT_SYMBOL(get_task_exe_file);
824
825 /**
826 * get_task_mm - acquire a reference to the task's mm
827 *
828 * Returns %NULL if the task has no mm. Checks PF_KTHREAD (meaning
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 47053 bytes --]
^ permalink raw reply
* [PATCH 2/2] audit: fix exe_file access in audit_exe_compare
From: Mateusz Guzik @ 2016-08-22 20:51 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
In-Reply-To: <1471899083-7937-1-git-send-email-mguzik@redhat.com>
Prior to the change the function would blindly deference mm, exe_file
and exe_file->f_inode, each of which could have been NULL or freed.
Use get_task_exe_file to safely obtain stable exe_file.
Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
kernel/audit_watch.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index d6709eb..0d302a8 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/file.h>
#include <linux/kernel.h>
#include <linux/audit.h>
#include <linux/kthread.h>
@@ -544,10 +545,11 @@ int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
unsigned long ino;
dev_t dev;
- rcu_read_lock();
- exe_file = rcu_dereference(tsk->mm->exe_file);
+ exe_file = get_task_exe_file(tsk);
+ if (!exe_file)
+ return 0;
ino = exe_file->f_inode->i_ino;
dev = exe_file->f_inode->i_sb->s_dev;
- rcu_read_unlock();
+ fput(exe_file);
return audit_mark_compare(mark, ino, dev);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH 1/2] mm: introduce get_task_exe_file
From: Mateusz Guzik @ 2016-08-22 20:51 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
In-Reply-To: <1471899083-7937-1-git-send-email-mguzik@redhat.com>
For more convenient access if one has a pointer to the task.
As a minor nit take advantage of the fact that only task lock + rcu are
needed to safely grab ->exe_file. This saves mm refcount dance.
Use the helper in proc_exe_link.
Signed-off-by: Mateusz Guzik <mguzik@redhat.com>
---
fs/proc/base.c | 7 +------
include/linux/mm.h | 1 +
kernel/fork.c | 24 ++++++++++++++++++++++++
3 files changed, 26 insertions(+), 6 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 2ed41cb..ebccdc1 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1556,18 +1556,13 @@ static const struct file_operations proc_pid_set_comm_operations = {
static int proc_exe_link(struct dentry *dentry, struct path *exe_path)
{
struct task_struct *task;
- struct mm_struct *mm;
struct file *exe_file;
task = get_proc_task(d_inode(dentry));
if (!task)
return -ENOENT;
- mm = get_task_mm(task);
+ exe_file = get_task_exe_file(task);
put_task_struct(task);
- if (!mm)
- return -ENOENT;
- exe_file = get_mm_exe_file(mm);
- mmput(mm);
if (exe_file) {
*exe_path = exe_file->f_path;
path_get(&exe_file->f_path);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9d85402..f4e639e 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2014,6 +2014,7 @@ extern void mm_drop_all_locks(struct mm_struct *mm);
extern void set_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file);
extern struct file *get_mm_exe_file(struct mm_struct *mm);
+extern struct file *get_task_exe_file(struct task_struct *task);
extern bool may_expand_vm(struct mm_struct *, vm_flags_t, unsigned long npages);
extern void vm_stat_account(struct mm_struct *, vm_flags_t, long npages);
diff --git a/kernel/fork.c b/kernel/fork.c
index 6fe775c..84a636d 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -800,6 +800,30 @@ struct file *get_mm_exe_file(struct mm_struct *mm)
EXPORT_SYMBOL(get_mm_exe_file);
/**
+ * get_task_exe_file - acquire a reference to the task's executable file
+ *
+ * Returns %NULL if task's mm (if any) has no associated executable file or
+ * this is a kernel thread with borrowed mm (see the comment above get_task_mm).
+ * User must release file via fput().
+ */
+struct file *get_task_exe_file(struct task_struct *task)
+{
+ struct file *exe_file = NULL;
+ struct mm_struct *mm;
+
+ task_lock(task);
+ mm = task->mm;
+ if (mm) {
+ if (!(task->flags & PF_KTHREAD))
+ exe_file = get_mm_exe_file(mm);
+ }
+ task_unlock(task);
+out:
+ return exe_file;
+}
+EXPORT_SYMBOL(get_task_exe_file);
+
+/**
* get_task_mm - acquire a reference to the task's mm
*
* Returns %NULL if the task has no mm. Checks PF_KTHREAD (meaning
--
1.8.3.1
^ permalink raw reply related
* [PATCH 0/2] introduce get_task_exe_file and use it to fix audit_exe_compare
From: Mateusz Guzik @ 2016-08-22 20:51 UTC (permalink / raw)
To: Konstantin Khlebnikov, Richard Guy Briggs
Cc: ebiederm, oleg, sgrubb, pmoore, eparis, luto, linux-audit,
linux-kernel, Al Viro
audit_exe_compare directly accesses mm->exe_file without making sure the
object is stable. Fixing it using current primitives results in
partially duplicating what proc_exe_link is doing.
As such, introduce a trivial helper which can be used in both places and
fix the func.
Mateusz Guzik (2):
mm: introduce get_task_exe_file
audit: fix exe_file access in audit_exe_compare
fs/proc/base.c | 7 +------
include/linux/mm.h | 1 +
kernel/audit_watch.c | 8 +++++---
kernel/fork.c | 24 ++++++++++++++++++++++++
4 files changed, 31 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply
* Re: [PATCH] audit: fix audit_exe_compare using get_mm_exe_file
From: Mateusz Guzik @ 2016-08-22 16:27 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: linux-audit, linux-kernel, ebiederm, oleg, skinsbursky, luto,
sgrubb, pmoore, eparis
In-Reply-To: <843cd1056b76a3c28585e5b6d711fe53d4f78c18.1471879964.git.rgb@redhat.com>
On Mon, Aug 22, 2016 at 11:41:34AM -0400, Richard Guy Briggs wrote:
> Fix original naive attempt to get/lock access to task->mm->exe_file by
> using get_mm_exe_file and checking for NULL.
>
> See: https://lkml.org/lkml/2016/7/30/97
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> kernel/audit_watch.c | 13 ++++++++++---
> 1 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index d6709eb..0b29279 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -19,6 +19,7 @@
> * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> */
>
> +#include <linux/file.h>
> #include <linux/kernel.h>
> #include <linux/audit.h>
> #include <linux/kthread.h>
> @@ -540,14 +541,20 @@ int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
>
> int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
> {
> + struct mm_struct *mm;
> struct file *exe_file;
> unsigned long ino;
> dev_t dev;
>
> - rcu_read_lock();
> - exe_file = rcu_dereference(tsk->mm->exe_file);
> + mm = get_task_mm(tsk);
> + if (!mm)
> + return 0;
> + exe_file = get_mm_exe_file(mm);
> + mmput(mm);
> + if (!exe_file)
> + return 0;
> ino = exe_file->f_inode->i_ino;
> dev = exe_file->f_inode->i_sb->s_dev;
> - rcu_read_unlock();
> + fput(exe_file);
> return audit_mark_compare(mark, ino, dev);
> }
I think this works but I have a better idea. This basically duplicates a
part of proc_exe_link and does unnecessary work.
Instead, one can introduce get_task_exe_file. task_lock held guarantees
the stability of mm struct itself, and then we can we can try to grab
the file without mm refcount dance.
I'll implement this later today.
--
Mateusz Guzik
^ permalink raw reply
* [PATCH] audit: fix audit_exe_compare using get_mm_exe_file
From: Richard Guy Briggs @ 2016-08-22 15:41 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, mguzik, ebiederm, oleg, skinsbursky, luto,
sgrubb, pmoore, eparis
Fix original naive attempt to get/lock access to task->mm->exe_file by
using get_mm_exe_file and checking for NULL.
See: https://lkml.org/lkml/2016/7/30/97
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit_watch.c | 13 ++++++++++---
1 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index d6709eb..0b29279 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -19,6 +19,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
+#include <linux/file.h>
#include <linux/kernel.h>
#include <linux/audit.h>
#include <linux/kthread.h>
@@ -540,14 +541,20 @@ int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark *mark)
{
+ struct mm_struct *mm;
struct file *exe_file;
unsigned long ino;
dev_t dev;
- rcu_read_lock();
- exe_file = rcu_dereference(tsk->mm->exe_file);
+ mm = get_task_mm(tsk);
+ if (!mm)
+ return 0;
+ exe_file = get_mm_exe_file(mm);
+ mmput(mm);
+ if (!exe_file)
+ return 0;
ino = exe_file->f_inode->i_ino;
dev = exe_file->f_inode->i_sb->s_dev;
- rcu_read_unlock();
+ fput(exe_file);
return audit_mark_compare(mark, ino, dev);
}
--
1.7.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox