* 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
* Re: [PATCH] prctl: remove one-shot limitation for changing exe link
From: Richard Guy Briggs @ 2016-08-22 15:40 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Mateusz Guzik, Cyrill Gorcunov, Stanislav Kinsburskiy, peterz,
mingo, mhocko, keescook, linux-kernel, bsegall, john.stultz, oleg,
matthltc, akpm, luto, vbabka, xemul, pmoore, linux-audit
In-Reply-To: <87vazlzlxm.fsf@x220.int.ebiederm.org>
On 2016-07-31 13:45, Eric W. Biederman wrote:
> Mateusz Guzik <mguzik@redhat.com> writes:
>
> > On Sat, Jul 30, 2016 at 12:31:40PM -0500, Eric W. Biederman wrote:
> >> So what I am requesting is very simple. That the checks in
> >> prctl_set_mm_exe_file be tightened up to more closely approach what
> >> execve requires. Thus preserving the value of the /proc/[pid]/exe for
> >> the applications that want to use the exe link.
> >>
> >> Once the checks in prctl_set_mm_exe_file are tightened up please feel
> >> free to remove the one shot test.
> >
> > This is more fishy.
> >
> > First of all exe_file is used by the audit subsystem. So someone has to
> > ask audit people what is the significance (if any) of the field.
This was added as part of the ability to audit execution by filename
rather than by inode, the latter of which must exist at the time of the
rule instantiation and can be renamed on disk. The former allows a rule
to be instantiated before the path exists and to follow the path even if
the original inode of the path is replaced.
> > All exe_file users but one use get_mm_exe_file and handle NULL
> > gracefully.
> >
> > Even with the current limit of changing the field once, the user can
> > cause a transient failure of get_mm_exe_file which can fail to increment
> > the refcount before it drops to 0.
> >
> > This transient failure can be used to get a NULL value stored in
> > ->exe_file during fork (in dup_mmap):
> > RCU_INIT_POINTER(mm->exe_file, get_mm_exe_file(oldmm));
> >
> > The one place which is not using get_mm_exe_file to get to the pointer
> > is audit_exe_compare:
> > rcu_read_lock();
> > exe_file = rcu_dereference(tsk->mm->exe_file);
> > ino = exe_file->f_inode->i_ino;
> > dev = exe_file->f_inode->i_sb->s_dev;
> > rcu_read_unlock();
> >
> > This is buggy on 2 accounts:
> > 1. exe_file can be NULL
Agreed, this is a bug.
> > 2. rcu does not protect f_inode
Thank you for pointing this out too.
I'll send a patch to fix this.
> > The issue is made worse with allowing arbitrary number changes.
> >
> > Modifying get_mm_exe_file to retry is trivial and in effect never return
> > NULL is trivial. With arbitrary number of changes allowed this may
> > require some cond_resched() or something.
I agree this sounds like a wise idea.
> > For comments I cc'ed Richard Guy Briggs, who is both an audit person and
> > the author of audit_exe_compare.
>
> That is fair. Keeping the existing users working is what needs to
> happen.
>
> At the same time we have an arbitrary number of possible changes with
> exec, but I guess that works differently because the mm is changed as
> well.
>
> So yes let's bug fix this piece of code and then we can see about
> relaxing constraints.
Ok, please comment on the subsequent patch and I'll get Paul Moore to
push this through the audit tree and also to stable.
> Eric
- 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: The default file for krb5_key_file is missing from the auditd.conf(5) manual
From: Mateusz Piotrowski @ 2016-08-21 19:49 UTC (permalink / raw)
To: linux-audit
In-Reply-To: <642023E9-D9DD-40AA-B4A0-15301F25FA70@FreeBSD.org>
Hello,
On 21 Aug 2016, at 21:00, Mateusz Piotrowski <0mp@freebsd.org> wrote:
> See this line[1]. It lacks the name of the default file.
>
> [1]: https://github.com/linux-audit/audit-userspace/blob/master/docs/auditd.conf.5#L291
I was able to fix this man page. Here's the patch:
>From e0650ae46d13ea9e588d2552c83513c554cf52dd Mon Sep 17 00:00:00 2001
From: Mateusz Piotrowski <mpp302@gmail.com>
Date: Sun, 21 Aug 2016 21:42:00 +0200
Subject: [PATCH] Fix auditd man page.
Related to this email: [The default file for krb5_key_file is missing
from the auditd.conf(5) manual][1]
[1]: https://www.redhat.com/archives/linux-audit/2016-August/msg00056.html.
---
docs/auditd.conf.5 | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/docs/auditd.conf.5 b/docs/auditd.conf.5
index 026a58d..1546b34 100644
--- a/docs/auditd.conf.5
+++ b/docs/auditd.conf.5
@@ -29,7 +29,7 @@ The log format describes how the information should be stored on disk. There are
the audit records will be stored in a format exactly as the kernel sends it. The
.IR ENRICHED
option will resolve all uid, gid, syscall, architecture, and socket address information before writing the event to disk. This aids in making sense of events created on one system but reported/analized on another system.
-The
+The
.I NOLOG
option is now deprecated. If you were setting this format, now you should set
the write_logs option to no.
@@ -256,7 +256,7 @@ events. In this case you would increase the number only large enough to let it
in too.
.TP
.I use_libwrap
-This setting determines whether or not to use tcp_wrappers to discern connection attempts that are from allowed machines. Legal values are either
+This setting determines whether or not to use tcp_wrappers to discern connection attempts that are from allowed machines. Legal values are either
.IR yes ", or " no "
The default value is yes.
.TP
@@ -288,12 +288,11 @@ server's host, as returned by a DNS lookup of its IP address.
.I krb5_key_file
Location of the key for this client's principal.
Note that the key file must be owned by root and mode 0400.
-The default is
+The default is \fI/etc/audit/audit.key\fP.
.TP
.I distribute_network
If set to "yes", network originating events will be distributed to the audit
dispatcher for processing. The default is "no".
-.I /etc/audit/audit.key
.SH NOTES
In a CAPP environment, the audit trail is considered so important that access to system resources must be denied if an audit trail cannot be created. In this environment, it would be suggested that /var/log/audit be on its own partition. This is to ensure that space detection is accurate and that no other process comes along and consumes part of it.
--
2.9.2
^ permalink raw reply related
* The default file for krb5_key_file is missing from the auditd.conf(5) manual
From: Mateusz Piotrowski @ 2016-08-21 19:00 UTC (permalink / raw)
To: linux-audit
Hello,
See this line[1]. It lacks the name of the default file.
As I don't know what the default file is I cannot submit a patch. Hopefully,
someone else can fix this file.
Cheers!
-Mateusz
[1]: https://github.com/linux-audit/audit-userspace/blob/master/docs/auditd.conf.5#L291
^ permalink raw reply
* Re: [PATCH V3 0/3] Add support for session ID user filtering
From: Paul Moore @ 2016-08-19 19:08 UTC (permalink / raw)
To: Richard Guy Briggs, sgrubb; +Cc: linux-audit, linux-kernel
In-Reply-To: <CAHC9VhR_ZOpdd1PWG9mqe=BmfL-10H8tBBc7FemwVeaj3EgnQw@mail.gmail.com>
On Thu, Aug 18, 2016 at 7:53 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Aug 18, 2016 at 1:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
>> RFE Session ID User Filter
>>
>> https://github.com/linux-audit/audit-kernel/issues/4
>> RFE: add a session ID filter to the kernel's user filter
>>
>> See also the set of userspace suport patches:
>> Add support for sessionid user filters, sessionid_set and loginuid_set
>> https://www.redhat.com/archives/linux-audit/2016-August/msg00005.html
>> (userspace update expected to be posted 2016-08-18)
>> and the test case:
>> https://github.com/rgbriggs/audit-testsuite/tree/ghak4-test-for-sessionID-user-filter
>>
>> This third patch is expected to have a merge conflict with:
>> "audit: add exclude filter extension to feature bitmap"
>> posted on 2016-08-18.
>>
>> Richard Guy Briggs (3):
>> audit: add support for session ID user filter
>> audit: add AUDIT_SESSIONID_SET support
>> audit: add sessionid filter extension to feature bitmap
>>
>> include/linux/audit.h | 10 ++++++++++
>> include/uapi/linux/audit.h | 6 +++++-
>> kernel/auditfilter.c | 5 +++++
>> kernel/auditsc.c | 6 ++++++
>> 4 files changed, 26 insertions(+), 1 deletions(-)
>
> These patches look fine to me; the only comment I have is that these
> should probably be combined into a single patch to avoid
> cherry-picking of individual pieces, e.g. skipping the feature bitmap
> or AUDIT_SESSION_SET support. I can do that when I merge the patches,
> no need to resend unless you really want to ...
>
> However, the bigger issue is coordination with the userspace patches.
> I really don't like merging kernel patches until Steve OK's the
> corresponding userspace patches.
I went ahead and squashed the patches into one and merged it into the
audit#working-session_filter-v3 branch. Take a look and if anything
looks awry let me know.
I'm also going to start including this patch/branch in my
pcmoore/kernel-secnext Copr builds so it is easier for you/sgrubb to
test the userspace support; once Steve OK's the userspace code I'll
merge this patch(set) into audit#next properly.
* https://github.com/linux-audit/audit-kernel/issues/4
* https://copr.fedorainfracloud.org/coprs/pcmoore/kernel-secnext
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V3 0/3] Add support for session ID user filtering
From: Paul Moore @ 2016-08-19 12:22 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: sgrubb, linux-audit, linux-kernel
In-Reply-To: <20160819043629.GA5983@madcap2.tricolour.ca>
On Fri, Aug 19, 2016 at 12:36 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-08-18 19:53, Paul Moore wrote:
>> These patches look fine to me; the only comment I have is that these
>> should probably be combined into a single patch to avoid
>> cherry-picking of individual pieces, e.g. skipping the feature bitmap
>> or AUDIT_SESSION_SET support. I can do that when I merge the patches,
>> no need to resend unless you really want to ...
>>
>> However, the bigger issue is coordination with the userspace patches.
>> I really don't like merging kernel patches until Steve OK's the
>> corresponding userspace patches.
>
> Well, some thought went in to making the two behave properly in the
> absence of an update of the other. This was the primary reason for the
> re-spin. That part of the process is working, since it was Steve's
> feedback that provoked the respin.
The issues isn't so much in making the different bits behave under
different circumstances - they need to do that period - the issue is
that I don't want to include new functionality in the kernel that
doesn't have a user (or one on the horizon).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH V3 0/3] Add support for session ID user filtering
From: Richard Guy Briggs @ 2016-08-19 4:36 UTC (permalink / raw)
To: Paul Moore; +Cc: sgrubb, linux-audit, linux-kernel
In-Reply-To: <CAHC9VhR_ZOpdd1PWG9mqe=BmfL-10H8tBBc7FemwVeaj3EgnQw@mail.gmail.com>
On 2016-08-18 19:53, Paul Moore wrote:
> On Thu, Aug 18, 2016 at 1:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
> > RFE Session ID User Filter
> >
> > https://github.com/linux-audit/audit-kernel/issues/4
> > RFE: add a session ID filter to the kernel's user filter
> >
> > See also the set of userspace suport patches:
> > Add support for sessionid user filters, sessionid_set and loginuid_set
> > https://www.redhat.com/archives/linux-audit/2016-August/msg00005.html
> > (userspace update expected to be posted 2016-08-18)
> > and the test case:
> > https://github.com/rgbriggs/audit-testsuite/tree/ghak4-test-for-sessionID-user-filter
> >
> > This third patch is expected to have a merge conflict with:
> > "audit: add exclude filter extension to feature bitmap"
> > posted on 2016-08-18.
> >
> > Richard Guy Briggs (3):
> > audit: add support for session ID user filter
> > audit: add AUDIT_SESSIONID_SET support
> > audit: add sessionid filter extension to feature bitmap
> >
> > include/linux/audit.h | 10 ++++++++++
> > include/uapi/linux/audit.h | 6 +++++-
> > kernel/auditfilter.c | 5 +++++
> > kernel/auditsc.c | 6 ++++++
> > 4 files changed, 26 insertions(+), 1 deletions(-)
>
> These patches look fine to me; the only comment I have is that these
> should probably be combined into a single patch to avoid
> cherry-picking of individual pieces, e.g. skipping the feature bitmap
> or AUDIT_SESSION_SET support. I can do that when I merge the patches,
> no need to resend unless you really want to ...
>
> However, the bigger issue is coordination with the userspace patches.
> I really don't like merging kernel patches until Steve OK's the
> corresponding userspace patches.
Well, some thought went in to making the two behave properly in the
absence of an update of the other. This was the primary reason for the
re-spin. That part of the process is working, since it was Steve's
feedback that provoked the respin.
> paul moore
- 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 V3 0/3] Add support for session ID user filtering
From: Paul Moore @ 2016-08-18 23:53 UTC (permalink / raw)
To: Richard Guy Briggs, sgrubb; +Cc: linux-audit, linux-kernel
In-Reply-To: <cover.1471541331.git.rgb@redhat.com>
On Thu, Aug 18, 2016 at 1:43 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> https://github.com/linux-audit/audit-kernel/wiki/RFE-Session-ID-User-Filter
> RFE Session ID User Filter
>
> https://github.com/linux-audit/audit-kernel/issues/4
> RFE: add a session ID filter to the kernel's user filter
>
> See also the set of userspace suport patches:
> Add support for sessionid user filters, sessionid_set and loginuid_set
> https://www.redhat.com/archives/linux-audit/2016-August/msg00005.html
> (userspace update expected to be posted 2016-08-18)
> and the test case:
> https://github.com/rgbriggs/audit-testsuite/tree/ghak4-test-for-sessionID-user-filter
>
> This third patch is expected to have a merge conflict with:
> "audit: add exclude filter extension to feature bitmap"
> posted on 2016-08-18.
>
> Richard Guy Briggs (3):
> audit: add support for session ID user filter
> audit: add AUDIT_SESSIONID_SET support
> audit: add sessionid filter extension to feature bitmap
>
> include/linux/audit.h | 10 ++++++++++
> include/uapi/linux/audit.h | 6 +++++-
> kernel/auditfilter.c | 5 +++++
> kernel/auditsc.c | 6 ++++++
> 4 files changed, 26 insertions(+), 1 deletions(-)
These patches look fine to me; the only comment I have is that these
should probably be combined into a single patch to avoid
cherry-picking of individual pieces, e.g. skipping the feature bitmap
or AUDIT_SESSION_SET support. I can do that when I merge the patches,
no need to resend unless you really want to ...
However, the bigger issue is coordination with the userspace patches.
I really don't like merging kernel patches until Steve OK's the
corresponding userspace patches.
--
paul moore
www.paul-moore.com
^ permalink raw reply
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