| Message ID | 1469544870-11574-1-git-send-email-jeffv@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
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 > 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))); > }
On 2016-08-17 16:58, 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? There is some naming confusion between userspace and kernel space with pid vs. tid vs. tgid... > 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? Adding fields in the middle isn't necessarily a problem if it doesn't confuse the existing scanner, which can skip over fields about which it does not care. I've carefully added fields in the middle in the past, trying my best to group it logically with the rest of the information as has been requested, I think: subject, action, object, result. > [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))); > > } > > -- > paul moore > www.paul-moore.com > > -- > Linux-audit mailing list > Linux-audit@redhat.com > https://www.redhat.com/mailman/listinfo/linux-audit - 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
On Thu, Aug 18, 2016 at 1:56 AM, Richard Guy Briggs <rgb@redhat.com> wrote: > On 2016-08-17 16:58, Paul Moore wrote: >> 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? > > Adding fields in the middle isn't necessarily a problem if it doesn't > confuse the existing scanner, which can skip over fields about which it > does not care. I've carefully added fields in the middle in the past, > trying my best to group it logically with the rest of the information as > has been requested, I think: subject, action, object, result. I've ranted about this before so I won't do it again here, but ultimately the problem is that the guidance for userspace applications/libraries has been that you can expect certain fields in specific locations.
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
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))); > > > > }
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 ;)
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(-) 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
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))); }
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(-)