* [ARCHIVE DEBUG 05/13] audit_debug: proc debug output easier to paste
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1445536765.git.rgb@redhat.com>
Convert /proc/audit debug output from label-value per line to one title
line with one output line.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 75 ++++++++++++++++++++++++++++++++++++++------------------
1 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index ffdec0c..30b3b08 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1188,30 +1188,57 @@ static struct pernet_operations audit_net_ops __net_initdata = {
/* Display information about audit subsystem */
static int proc_auditstats_show(struct seq_file *m, void *v)
{
- //seq_printf(m, "audit_initialized\t%d\n", audit_initialized);
- //seq_printf(m, "audit_enabled\t%u\n", audit_enabled);
- //seq_printf(m, "audit_ever_enabled\t%u\n", audit_ever_enabled);
- //seq_printf(m, "audit_default\t%u\n", audit_default);
- //seq_printf(m, "audit_failure\t%u\n", audit_failure);
- seq_printf(m, "audit_pid\t\t\t%d\n", audit_pid);
- //seq_printf(m, "audit_nlk_portid\t%u\n", audit_nlk_portid);
- //seq_printf(m, "audit_rate_limit\t%u\n", audit_rate_limit);
- //seq_printf(m, "audit_backlog_limit\t%u\n", audit_backlog_limit);
- //seq_printf(m, "audit_backlog_wait_time\t%u\n", audit_backlog_wait_time);
- //seq_printf(m, "audit_sig_uid\t%u\n", from_kuid(&init_user_ns, audit_sig_uid));
- //seq_printf(m, "audit_sig_pid\t%d\n", audit_sig_pid);
- //seq_printf(m, "audit_sig_sid\t%u\n", audit_sig_sid);
- seq_printf(m, "audit_lost\t\t\t%d\n", atomic_read(&audit_lost));
- seq_printf(m, "audit_hold_lost\t\t\t%d\n", atomic_read(&audit_hold_lost));
- seq_printf(m, "audit_freelist_count\t\t%u\n", audit_freelist_count);
- seq_printf(m, "audit_skb_queue len\t\t%d\n", skb_queue_len(&audit_skb_queue));
- seq_printf(m, "audit_skb_queue len_max\t\t%d\n", skb_queue_len_max(&audit_skb_queue));
- seq_printf(m, "audit_skb_hold_queue len\t%d\n", skb_queue_len(&audit_skb_hold_queue));
- seq_printf(m, "audit_skb_hold_queue len_max\t%d\n", skb_queue_len_max(&audit_skb_hold_queue));
- seq_printf(m, "audit_overflows\t\t\t%d\n", atomic_read(&audit_overflows));
- seq_printf(m, "audit_waiters\t\t\t%d\n", atomic_read(&audit_waiters));
- seq_printf(m, "audit_recovers\t\t\t%d\n", atomic_read(&audit_recovers));
- seq_printf(m, "audit_reserves\t\t\t%d\n", atomic_read(&audit_reserves));
+ //seq_printf(m, "init\t");
+ //seq_printf(m, "en\t");
+ //seq_printf(m, "ever_en\t");
+ //seq_printf(m, "def\t");
+ //seq_printf(m, "failure\t");
+ seq_printf(m, "pid\t");
+ //seq_printf(m, "portid\t");
+ //seq_printf(m, "rt_lim\t");
+ //seq_printf(m, "bk_lim\t");
+ //seq_printf(m, "bk_wait\t");
+ //seq_printf(m, "sig_uid\t");
+ //seq_printf(m, "sig_pid\t");
+ //seq_printf(m, "sig_sid\t");
+ seq_printf(m, "lost\t");
+ seq_printf(m, "hlost\t");
+ seq_printf(m, "freelis\t");
+ seq_printf(m, "Qln\t");
+ seq_printf(m, "Qln_mx\t");
+ seq_printf(m, "hQln\t");
+ seq_printf(m, "hQln_mx\t");
+ seq_printf(m, "oflow\t");
+ seq_printf(m, "waiter\t");
+ seq_printf(m, "recover\t");
+ seq_printf(m, "reserve\t");
+ seq_printf(m, "\n");
+
+ //seq_printf(m, "%d\t", audit_initialized);
+ //seq_printf(m, "%u\t", audit_enabled);
+ //seq_printf(m, "%u\t", audit_ever_enabled);
+ //seq_printf(m, "%u\t", audit_default);
+ //seq_printf(m, "%u\t", audit_failure);
+ seq_printf(m, "%d\t", audit_pid);
+ //seq_printf(m, "%u\t", audit_nlk_portid);
+ //seq_printf(m, "%u\t", audit_rate_limit);
+ //seq_printf(m, "%u\t", audit_backlog_limit);
+ //seq_printf(m, "%u\t", audit_backlog_wait_time);
+ //seq_printf(m, "%u\t", from_kuid(&init_user_ns, audit_sig_uid));
+ //seq_printf(m, "%d\t", audit_sig_pid);
+ //seq_printf(m, "%u\t", audit_sig_sid);
+ seq_printf(m, "%d\t", atomic_read(&audit_lost));
+ seq_printf(m, "%d\t", atomic_read(&audit_hold_lost));
+ seq_printf(m, "%u\t", audit_freelist_count);
+ seq_printf(m, "%d\t", skb_queue_len(&audit_skb_queue));
+ seq_printf(m, "%d\t", skb_queue_len_max(&audit_skb_queue));
+ seq_printf(m, "%d\t", skb_queue_len(&audit_skb_hold_queue));
+ seq_printf(m, "%d\t", skb_queue_len_max(&audit_skb_hold_queue));
+ seq_printf(m, "%d\t", atomic_read(&audit_overflows));
+ seq_printf(m, "%d\t", atomic_read(&audit_waiters));
+ seq_printf(m, "%d\t", atomic_read(&audit_recovers));
+ seq_printf(m, "%d\t", atomic_read(&audit_reserves));
+ seq_printf(m, "\n");
return 0;
}
--
1.7.1
^ permalink raw reply related
* [ARCHIVE DEBUG 04/13] audit_debug: add /proc counters for overflows, waiters, recovers, reserves
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1445536765.git.rgb@redhat.com>
Add counters to /proc/audit to instrument the wait_for_auditd condition.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 82df9fd..ffdec0c 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -125,6 +125,10 @@ u32 audit_sig_sid = 0;
*/
static atomic_t audit_lost = ATOMIC_INIT(0);
static atomic_t audit_hold_lost = ATOMIC_INIT(0);
+static atomic_t audit_overflows = ATOMIC_INIT(0);
+static atomic_t audit_waiters = ATOMIC_INIT(0);
+static atomic_t audit_recovers = ATOMIC_INIT(0);
+static atomic_t audit_reserves = ATOMIC_INIT(0);
/* The netlink socket. */
static struct sock *audit_sock;
@@ -1204,6 +1208,10 @@ static int proc_auditstats_show(struct seq_file *m, void *v)
seq_printf(m, "audit_skb_queue len_max\t\t%d\n", skb_queue_len_max(&audit_skb_queue));
seq_printf(m, "audit_skb_hold_queue len\t%d\n", skb_queue_len(&audit_skb_hold_queue));
seq_printf(m, "audit_skb_hold_queue len_max\t%d\n", skb_queue_len_max(&audit_skb_hold_queue));
+ seq_printf(m, "audit_overflows\t\t\t%d\n", atomic_read(&audit_overflows));
+ seq_printf(m, "audit_waiters\t\t\t%d\n", atomic_read(&audit_waiters));
+ seq_printf(m, "audit_recovers\t\t\t%d\n", atomic_read(&audit_recovers));
+ seq_printf(m, "audit_reserves\t\t\t%d\n", atomic_read(&audit_reserves));
return 0;
}
@@ -1422,6 +1430,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
/* reserve: Allow atomic callers to go over the normal backlog limit */
int reserve = audit_backlog_limit/16;
unsigned long timeout_start = jiffies;
+ int waiter = 0;
+ int overflow_counted = 0;
if (audit_initialized != AUDIT_INITIALIZED)
return NULL;
@@ -1434,7 +1444,10 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (ACCESS_ONCE(audit_cmd_mutex.owner) == current ||
current->tgid == 1 ||
(audit_pid && audit_pid == current->tgid))
+ {
gfp_mask &= ~__GFP_WAIT;
+ atomic_inc(&audit_reserves);
+ }
else
reserve = 0;
rcu_read_unlock();
@@ -1449,14 +1462,25 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
while (audit_backlog_limit
&& skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
+ if (!overflow_counted) {
+ atomic_inc(&audit_overflows);
+ overflow_counted = 1;
+ }
if (gfp_mask & __GFP_WAIT && audit_backlog_wait_time) {
long sleep_time;
+ if (!waiter) {
+ atomic_inc(&audit_waiters);
+ waiter = 1;
+ }
sleep_time = timeout_start + audit_backlog_wait_time - jiffies;
if (sleep_time > 0) {
sleep_time = wait_for_auditd(sleep_time);
if (audit_pid && sleep_time > 0)
+ {
+ atomic_inc(&audit_recovers);
continue;
+ }
}
}
if (audit_rate_check() && printk_ratelimit())
--
1.7.1
^ permalink raw reply related
* [ARCHIVE DEBUG 03/13] audit_debug: proc instrumentation
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1445536765.git.rgb@redhat.com>
Add a /proc/audit entry for debugging to instrument many audit subsystem
internal parameters not normally visible.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/skbuff.h | 16 ++++++++++++++++
kernel/audit.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+), 0 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index f54d665..bcde922 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -180,6 +180,7 @@ struct sk_buff_head {
__u32 qlen;
spinlock_t lock;
+ __u32 qlen_max;
};
struct sk_buff;
@@ -1301,6 +1302,11 @@ static inline __u32 skb_queue_len(const struct sk_buff_head *list_)
return list_->qlen;
}
+static inline __u32 skb_queue_len_max(const struct sk_buff_head *list_)
+{
+ return list_->qlen_max;
+}
+
/**
* __skb_queue_head_init - initialize non-spinlock portions of sk_buff_head
* @list: queue to initialize
@@ -1354,6 +1360,8 @@ static inline void __skb_insert(struct sk_buff *newsk,
newsk->prev = prev;
next->prev = prev->next = newsk;
list->qlen++;
+ if(list->qlen > list->qlen_max)
+ list->qlen_max = list->qlen;
}
static inline void __skb_queue_splice(const struct sk_buff_head *list,
@@ -1381,6 +1389,8 @@ static inline void skb_queue_splice(const struct sk_buff_head *list,
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, (struct sk_buff *) head, head->next);
head->qlen += list->qlen;
+ if(head->qlen > head->qlen_max)
+ head->qlen_max = head->qlen;
}
}
@@ -1397,6 +1407,8 @@ static inline void skb_queue_splice_init(struct sk_buff_head *list,
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, (struct sk_buff *) head, head->next);
head->qlen += list->qlen;
+ if(head->qlen > head->qlen_max)
+ head->qlen_max = head->qlen;
__skb_queue_head_init(list);
}
}
@@ -1412,6 +1424,8 @@ static inline void skb_queue_splice_tail(const struct sk_buff_head *list,
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, head->prev, (struct sk_buff *) head);
head->qlen += list->qlen;
+ if(head->qlen > head->qlen_max)
+ head->qlen_max = head->qlen;
}
}
@@ -1429,6 +1443,8 @@ static inline void skb_queue_splice_tail_init(struct sk_buff_head *list,
if (!skb_queue_empty(list)) {
__skb_queue_splice(list, head->prev, (struct sk_buff *) head);
head->qlen += list->qlen;
+ if(head->qlen > head->qlen_max)
+ head->qlen_max = head->qlen;
__skb_queue_head_init(list);
}
}
diff --git a/kernel/audit.c b/kernel/audit.c
index d4e19fc..82df9fd 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -124,6 +124,7 @@ u32 audit_sig_sid = 0;
4) suppressed due to audit_backlog_limit
*/
static atomic_t audit_lost = ATOMIC_INIT(0);
+static atomic_t audit_hold_lost = ATOMIC_INIT(0);
/* The netlink socket. */
static struct sock *audit_sock;
@@ -381,7 +382,10 @@ static void audit_hold_skb(struct sk_buff *skb)
skb_queue_len(&audit_skb_hold_queue) < audit_backlog_limit))
skb_queue_tail(&audit_skb_hold_queue, skb);
else
+ {
kfree_skb(skb);
+ atomic_inc(&audit_hold_lost);
+ }
}
/*
@@ -510,7 +514,10 @@ static void flush_hold_queue(void)
* dequeued an skb we need to drop ref
*/
if (skb)
+ {
consume_skb(skb);
+ atomic_inc(&audit_hold_lost);
+ }
}
static int kauditd_thread(void *dummy)
@@ -1174,6 +1181,45 @@ static struct pernet_operations audit_net_ops __net_initdata = {
.size = sizeof(struct audit_net),
};
+/* Display information about audit subsystem */
+static int proc_auditstats_show(struct seq_file *m, void *v)
+{
+ //seq_printf(m, "audit_initialized\t%d\n", audit_initialized);
+ //seq_printf(m, "audit_enabled\t%u\n", audit_enabled);
+ //seq_printf(m, "audit_ever_enabled\t%u\n", audit_ever_enabled);
+ //seq_printf(m, "audit_default\t%u\n", audit_default);
+ //seq_printf(m, "audit_failure\t%u\n", audit_failure);
+ seq_printf(m, "audit_pid\t\t\t%d\n", audit_pid);
+ //seq_printf(m, "audit_nlk_portid\t%u\n", audit_nlk_portid);
+ //seq_printf(m, "audit_rate_limit\t%u\n", audit_rate_limit);
+ //seq_printf(m, "audit_backlog_limit\t%u\n", audit_backlog_limit);
+ //seq_printf(m, "audit_backlog_wait_time\t%u\n", audit_backlog_wait_time);
+ //seq_printf(m, "audit_sig_uid\t%u\n", from_kuid(&init_user_ns, audit_sig_uid));
+ //seq_printf(m, "audit_sig_pid\t%d\n", audit_sig_pid);
+ //seq_printf(m, "audit_sig_sid\t%u\n", audit_sig_sid);
+ seq_printf(m, "audit_lost\t\t\t%d\n", atomic_read(&audit_lost));
+ seq_printf(m, "audit_hold_lost\t\t\t%d\n", atomic_read(&audit_hold_lost));
+ seq_printf(m, "audit_freelist_count\t\t%u\n", audit_freelist_count);
+ seq_printf(m, "audit_skb_queue len\t\t%d\n", skb_queue_len(&audit_skb_queue));
+ seq_printf(m, "audit_skb_queue len_max\t\t%d\n", skb_queue_len_max(&audit_skb_queue));
+ seq_printf(m, "audit_skb_hold_queue len\t%d\n", skb_queue_len(&audit_skb_hold_queue));
+ seq_printf(m, "audit_skb_hold_queue len_max\t%d\n", skb_queue_len_max(&audit_skb_hold_queue));
+
+ return 0;
+}
+
+static int auditstats_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, proc_auditstats_show, NULL);
+}
+
+static const struct file_operations proc_auditstats_operations = {
+ .open = auditstats_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
/* Initialize audit support at boot time. */
static int __init audit_init(void)
{
@@ -1197,6 +1243,8 @@ static int __init audit_init(void)
for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
INIT_LIST_HEAD(&audit_inode_hash[i]);
+ proc_create("audit", 0, NULL, &proc_auditstats_operations);
+
return 0;
}
__initcall(audit_init);
--
1.7.1
^ permalink raw reply related
* [ARCHIVE DEBUG 02/13] audit_debug: squawk when digging into reserve
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1445536765.git.rgb@redhat.com>
Debug when audit_backlog_limit reserves are used for bypassing the queue
limits by listing timing, process and queue length.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 795d604..d4e19fc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1391,6 +1391,14 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
reserve = 0;
rcu_read_unlock();
}
+
+ if (reserve) {
+ pr_warn("( %lu %lu )"
+ " Qreq apid=%d pid=%d comm=%s type=%d reserve=%d Qlen=%d\n",
+ timeout_start, jiffies-timeout_start,
+ audit_pid, current->pid, current->comm, type, reserve, skb_queue_len(&audit_skb_queue));
+ }
+
while (audit_backlog_limit
&& skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
if (gfp_mask & __GFP_WAIT && audit_backlog_wait_time) {
--
1.7.1
^ permalink raw reply related
* [ARCHIVE DEBUG 01/13] audit_debug: set reserve to same size as backlog
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
In-Reply-To: <cover.1445536765.git.rgb@redhat.com>
Debug the unlikely possibility that the audit_backlog_limit reserve is
being overflowed too. Make the reserve proportional to the backlog
limit.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 369cfcc..795d604 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1371,8 +1371,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
struct audit_buffer *ab = NULL;
struct timespec t;
unsigned int uninitialized_var(serial);
- int reserve = 5; /* Allow atomic callers to go up to five
- entries over the normal backlog limit */
+ /* reserve: Allow atomic callers to go over the normal backlog limit */
+ int reserve = audit_backlog_limit/16;
unsigned long timeout_start = jiffies;
if (audit_initialized != AUDIT_INITIALIZED)
--
1.7.1
^ permalink raw reply related
* [ARCHIVE DEBUG 00/13] audit_debug: diagnostics for audit internal workings
From: Richard Guy Briggs @ 2015-10-22 18:58 UTC (permalink / raw)
To: linux-audit; +Cc: Richard Guy Briggs
A number of diagnostic patches including a /proc/audit* interface to get
a better view of what is happenning with what frequency in the audit
queue code.
Richard Guy Briggs (13):
audit_debug: set reserve to same size as backlog
audit_debug: squawk when digging into reserve
audit_debug: proc instrumentation
audit_debug: add /proc counters for overflows, waiters, recovers,
reserves
audit_debug: proc debug output easier to paste
audit_debug: don't let systemd change config
audit_debug: instrument audit_cmd_mutex contention
audit_debug: seperate out audit_titles in proc display
netlink_debug: instrument timing of calls by init/systemd or
audit_pid
audit_debug: bark on impossible reserve process conditions in while
loop
audit_debug: instrument audit_receive timing
audit_debug: list processes and frequencies waiting for auditd
audit_debug: print inode nr for each path when listing exe rules
include/linux/skbuff.h | 16 ++++
kernel/audit.c | 219 ++++++++++++++++++++++++++++++++++++++++++++--
kernel/audit.h | 2 +
kernel/audit_fsnotify.c | 5 +
kernel/auditfilter.c | 1 +
net/netlink/af_netlink.c | 16 ++++
6 files changed, 250 insertions(+), 9 deletions(-)
^ permalink raw reply
* [RFC PATCH 7/7] audit: wake up kauditd_thread after auditd registers
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, sgrubb, pmoore, eparis, v.rathor, ctcard
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
When auditd is restarted, even though the kauditd_thread is present, it
remains dormant until the next audit log message is queued.
Wake up the kauditd_thread in the kauditd_wait queue immediately when
auditd registers its availability to drain the queue.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 688fa1e..369cfcc 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -885,6 +885,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
audit_sock = skb->sk;
if (!audit_pid)
wake_up(&audit_backlog_wait);
+ if (audit_pid)
+ wake_up_interruptible(&kauditd_wait);
}
if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
err = audit_set_rate_limit(s.rate_limit);
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 6/7] audit: wake up audit_backlog_wait queue when auditd goes away.
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
When auditd goes away (died, killed or shutdown, or net namespace shut
down), there is no point in sleeping waiting for auditd to drain the
queue since that message would be distined for the hold queue after the
timeout anyways. This will needlessly have those processes wait the
full default timeout of 60 seconds (audit_backlog_wait_time).
Wake up the processes caught in the audit_backlog_wait queue when auditd
is no longer present so they can be sent instead to the hold queue.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 34411af..688fa1e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -425,6 +425,7 @@ restart:
audit_log_lost(s);
audit_pid = 0;
audit_sock = NULL;
+ wake_up(&audit_backlog_wait);
} else {
pr_warn("re-scheduling(#%d) write to audit_pid=%d\n",
attempts, audit_pid);
@@ -882,6 +883,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
audit_sock = skb->sk;
+ if (!audit_pid)
+ wake_up(&audit_backlog_wait);
}
if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
err = audit_set_rate_limit(s.rate_limit);
@@ -1154,6 +1157,7 @@ static void __net_exit audit_net_exit(struct net *net)
if (sock == audit_sock) {
audit_pid = 0;
audit_sock = NULL;
+ wake_up(&audit_backlog_wait);
}
RCU_INIT_POINTER(aunet->nlsk, NULL);
@@ -1393,7 +1397,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
sleep_time = timeout_start + audit_backlog_wait_time - jiffies;
if (sleep_time > 0) {
sleep_time = wait_for_auditd(sleep_time);
- if (sleep_time > 0)
+ if (audit_pid && sleep_time > 0)
continue;
}
}
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 5/7] audit: allow audit_cmd_mutex holders to use reserves
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
If we hold the audit_cmd_mutex, we should never sleep waiting for auditd
to drain the queue since auditd may need the mutex to shut down.
This was first implemented with mutex_trylock(), but since
audit_log_start() can be called in softirq context, that won't work.
Next, owner_running() was used to check audit_cmd_mutex but another
process could have this locked on another cpu. Use rcu_read_lock() and
ACCESS_ONCE() to check audit_cmd_mutex.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 02a5ec0..34411af 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1376,12 +1376,15 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
return NULL;
if (gfp_mask & __GFP_WAIT) {
- if (current->tgid == 1 || (audit_pid && audit_pid == current->tgid))
+ rcu_read_lock();
+ if (ACCESS_ONCE(audit_cmd_mutex.owner) == current ||
+ current->tgid == 1 ||
+ (audit_pid && audit_pid == current->tgid))
gfp_mask &= ~__GFP_WAIT;
else
reserve = 0;
+ rcu_read_unlock();
}
-
while (audit_backlog_limit
&& skb_queue_len(&audit_skb_queue) > audit_backlog_limit + reserve) {
if (gfp_mask & __GFP_WAIT && audit_backlog_wait_time) {
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 4/7] audit: wake up threads if queue switched from limited to unlimited
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
If the audit_backlog_limit is changed from a limited value to an
unlimited value (zero) while the queue was overflowed, wake up the
audit_backlog_wait queue to allow those processes to continue.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 384a1a1..02a5ec0 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -523,7 +523,8 @@ static int kauditd_thread(void *dummy)
skb = skb_dequeue(&audit_skb_queue);
if (skb) {
- if (skb_queue_len(&audit_skb_queue) <= audit_backlog_limit)
+ if (!audit_backlog_limit ||
+ (skb_queue_len(&audit_skb_queue) <= audit_backlog_limit))
wake_up(&audit_backlog_wait);
if (audit_pid)
kauditd_send_skb(skb);
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 3/7] audit: allow systemd to use queue reserves
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
Treat systemd the same way as auditd, allowing it to overrun the queue to avoid
blocking.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 3917aad..384a1a1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
return NULL;
if (gfp_mask & __GFP_WAIT) {
- if (audit_pid && audit_pid == current->tgid)
+ if (current->tgid == 1 || (audit_pid && audit_pid == current->tgid))
gfp_mask &= ~__GFP_WAIT;
else
reserve = 0;
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 2/7] audit: include auditd's threads in audit_log_start() wait exception
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel
Cc: Richard Guy Briggs, sgrubb, pmoore, eparis, v.rathor, ctcard
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
Should auditd spawn threads, allow all members of its thread group to
use the audit_backlog_limit reserves to bypass the queue limits too.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index daefd81..3917aad 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1375,7 +1375,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
return NULL;
if (gfp_mask & __GFP_WAIT) {
- if (audit_pid && audit_pid == current->pid)
+ if (audit_pid && audit_pid == current->tgid)
gfp_mask &= ~__GFP_WAIT;
else
reserve = 0;
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 1/7] audit: don't needlessly reset valid wait time
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
In-Reply-To: <cover.1445539473.git.rgb@redhat.com>
After auditd has recovered from an overflowed queue, the first process
that doesn't use reserves to make it through the queue checks should
reset the audit backlog wait time to the configured value. After that,
there is no need to keep resetting it.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
kernel/audit.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index a72ad37..daefd81 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1403,7 +1403,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
return NULL;
}
- if (!reserve)
+ if (!reserve && !audit_backlog_wait_time)
audit_backlog_wait_time = audit_backlog_wait_time_master;
ab = audit_buffer_alloc(ctx, gfp_mask, type);
--
1.7.1
^ permalink raw reply related
* [RFC PATCH 0/7] audit: clean up audit queue handling
From: Richard Guy Briggs @ 2015-10-22 18:53 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: v.rathor, Richard Guy Briggs
This set of patches cleans up a number of corner cases in the management
of the audit queue.
Richard Guy Briggs (7):
audit: don't needlessly reset valid wait time
audit: include auditd's threads in audit_log_start() wait exception
audit: allow systemd to use queue reserves
audit: wake up threads if queue switched from limited to unlimited
audit: allow audit_cmd_mutex holders to use reserves
audit: wake up audit_backlog_wait queue when auditd goes away.
audit: wake up kauditd_thread after auditd registers
kernel/audit.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
^ permalink raw reply
* BSides Portland - The Linux Audit Framework
From: Smith, Gary R @ 2015-10-22 15:53 UTC (permalink / raw)
To: linux-audit@redhat.com
[-- Attachment #1.1: Type: text/plain, Size: 811 bytes --]
Hello All,
I was at the BSides Portland security conference last weekend and I gave a presentation called “The Linux Audit Framework” there. I have put up the slides from the presentation on slideshare. I have also put up a file that implements the Center for Internet Security RHEL 6 Benchmark for audit rules. In addition, I have put up a document that is “Smith’s Audit Cheat Sheet”. It is what audit commands to run by analysts during first, second and third shift.
The presentation slides are at: http://www.slideshare.net/GarySmith99/the-linux-audit-framework
The CIS audit rules are at: http://www.slideshare.net/GarySmith99/linux-auditrules
The “Cheat Sheet” are at: http://www.slideshare.net/GarySmith99/audit-commands-by-shift
I hope you find them useful.
Best regards,
Gary Smith
[-- Attachment #1.2: Type: text/html, Size: 1541 bytes --]
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Richard Guy Briggs @ 2015-10-22 12:53 UTC (permalink / raw)
To: Scott Matheina
Cc: Joe Perches, Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <562857B2.4070408@matheina.com>
On 15/10/21, Scott Matheina wrote:
> On 10/21/2015 09:15 PM, Richard Guy Briggs wrote:
> > On 15/10/21, Scott Matheina wrote:
> >> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> >>> On 15/10/21, Joe Perches wrote:
> >>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> >>>>> On 15/10/18, Scott Matheina wrote:
> >>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
> >>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >>>> []
> >>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >>>> []
> >>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> >>>>>>>> {
> >>>>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> >>>>>>>> audit_free_rule(e);
> >>>>>>>> +
> >>>>>>>> }
> >>>>>>> Why?
> >>>>>> I was following the error messages in checkpatch.pl, but the warning
> >>>>>> went away after adding this line. No problem with the code.
> >>>>> That sounds like a bug in checkpatch.pl, since that blank line should be
> >>>>> tween the declaration and the function call.
> >>>> checkpatch message asks for a blank line after the
> >>>> "struct audit_entry *e = ..." declaration.
> >>> Well then maybe it is a bug in his interpretation of the output of
> >>> checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those
> >>> spaces? Did it pass?
> >> The error did go away.
> > Joe, I confirm the error went away. Looks like a bug in checkpatch.pl
> > to me. I tried a number of combinations of things and it didn't
> > complain about several things it should have. I did try a few other
> > things to make sure it was still finding problems like brace placement
> > and leading spaces, but it looks like the blank line checking code isn't
> > working. This is on 4.0, so maybe it has been fixed since then. Scott,
> > what kernel version are you using?
> I'm running Ubuntu 14.04 LTS (Kernel 3.19.0-30-generic) on my machine.
>
> I cloned Linus' repo for source code. I'm pretty sure you were talking
> about the active Kernel on my machine, so if not please let me know.
I was talking about the source used to generate this patch in question,
run ./scripts/checkpatch.pl and do a compile test. The active kernel on
your machine is irrelevant unless you subsequently booted it to test it.
How recent is your clone/pull of Linus' repo?
> >>>>>>>> while (*list != ~0U) {
> >>>>>>>> +
> >>>>>>>> unsigned n = *list++;
> >>>>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> >>>>>>>> kfree(p);
> >>>>>>> Why?
> >>>>>> This is the same as above. Just going through the checkpatch.pl
> >>>>>> script, and looking for warnings to fix.
> >>>>> Again, another manifestation of that bug? That blank line should be
> >>>>> after the declaration and before the if statement.
> >>>> []
> >>>>> Well, I agree, you have to start somewhere... Too bad you hit a bug in
> >>>>> checkpatch.pl!
> >>>> Here too, not a bug in checkpatch.
> >>>>
> >>>> checkpatch output asks for a blank line after the
> >>>> "unsigned n" declaration, not before.
> >>> - RGB
> > - RGB
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Scott Matheina @ 2015-10-22 3:27 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Joe Perches, Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <20151022021500.GD5269@madcap2.tricolour.ca>
On 10/21/2015 09:15 PM, Richard Guy Briggs wrote:
> On 15/10/21, Scott Matheina wrote:
>> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
>>> On 15/10/21, Joe Perches wrote:
>>>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
>>>>> On 15/10/18, Scott Matheina wrote:
>>>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>>>> []
>>>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> []
>>>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>>>>>>> {
>>>>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>>>>>>> audit_free_rule(e);
>>>>>>>> +
>>>>>>>> }
>>>>>>> Why?
>>>>>> I was following the error messages in checkpatch.pl, but the warning
>>>>>> went away after adding this line. No problem with the code.
>>>>> That sounds like a bug in checkpatch.pl, since that blank line should be
>>>>> tween the declaration and the function call.
>>>> checkpatch message asks for a blank line after the
>>>> "struct audit_entry *e = ..." declaration.
>>> Well then maybe it is a bug in his interpretation of the output of
>>> checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those
>>> spaces? Did it pass?
>> The error did go away.
> Joe, I confirm the error went away. Looks like a bug in checkpatch.pl
> to me. I tried a number of combinations of things and it didn't
> complain about several things it should have. I did try a few other
> things to make sure it was still finding problems like brace placement
> and leading spaces, but it looks like the blank line checking code isn't
> working. This is on 4.0, so maybe it has been fixed since then. Scott,
> what kernel version are you using?
I'm running Ubuntu 14.04 LTS (Kernel 3.19.0-30-generic) on my machine.
I cloned Linus' repo for source code. I'm pretty sure you were talking about the active Kernel on my machine, so if not please let me know.
>
>>>>>>>> while (*list != ~0U) {
>>>>>>>> +
>>>>>>>> unsigned n = *list++;
>>>>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>>>>>>> kfree(p);
>>>>>>> Why?
>>>>>> This is the same as above. Just going through the checkpatch.pl
>>>>>> script, and looking for warnings to fix.
>>>>> Again, another manifestation of that bug? That blank line should be
>>>>> after the declaration and before the if statement.
>>>> []
>>>>> Well, I agree, you have to start somewhere... Too bad you hit a bug in
>>>>> checkpatch.pl!
>>>> Here too, not a bug in checkpatch.
>>>>
>>>> checkpatch output asks for a blank line after the
>>>> "unsigned n" declaration, not before.
>>> - RGB
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Richard Guy Briggs @ 2015-10-22 2:15 UTC (permalink / raw)
To: Scott Matheina
Cc: Joe Perches, Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <56282E2E.4010207@matheina.com>
On 15/10/21, Scott Matheina wrote:
> On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> > On 15/10/21, Joe Perches wrote:
> >> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> >>> On 15/10/18, Scott Matheina wrote:
> >>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
> >>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >> []
> >>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> []
> >>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> >>>>>> {
> >>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> >>>>>> audit_free_rule(e);
> >>>>>> +
> >>>>>> }
> >>>>> Why?
> >>>> I was following the error messages in checkpatch.pl, but the warning
> >>>> went away after adding this line. No problem with the code.
> >>> That sounds like a bug in checkpatch.pl, since that blank line should be
> >>> tween the declaration and the function call.
> >> checkpatch message asks for a blank line after the
> >> "struct audit_entry *e = ..." declaration.
> > Well then maybe it is a bug in his interpretation of the output of
> > checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those
> > spaces? Did it pass?
>
> The error did go away.
Joe, I confirm the error went away. Looks like a bug in checkpatch.pl
to me. I tried a number of combinations of things and it didn't
complain about several things it should have. I did try a few other
things to make sure it was still finding problems like brace placement
and leading spaces, but it looks like the blank line checking code isn't
working. This is on 4.0, so maybe it has been fixed since then. Scott,
what kernel version are you using?
> >>>>>> while (*list != ~0U) {
> >>>>>> +
> >>>>>> unsigned n = *list++;
> >>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> >>>>>> kfree(p);
> >>>>> Why?
> >>>> This is the same as above. Just going through the checkpatch.pl
> >>>> script, and looking for warnings to fix.
> >>> Again, another manifestation of that bug? That blank line should be
> >>> after the declaration and before the if statement.
> >> []
> >>> Well, I agree, you have to start somewhere... Too bad you hit a bug in
> >>> checkpatch.pl!
> >> Here too, not a bug in checkpatch.
> >>
> >> checkpatch output asks for a blank line after the
> >> "unsigned n" declaration, not before.
> > - RGB
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Scott Matheina @ 2015-10-22 0:35 UTC (permalink / raw)
To: Paul Moore; +Cc: eparis, linux-audit, linux-kernel, trivial
In-Reply-To: <20081879.d8apxLOWgP@sifl>
On 10/21/2015 01:23 PM, Paul Moore wrote:
> On Sunday, October 18, 2015 12:50:45 PM Scott Matheina wrote:
>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>>>> Signed-off-by: Scott Matheina <scott@matheina.com>
>>>> ---
>>>>
>>>> kernel/auditfilter.c | 17 ++++++++++-------
>>>> 1 file changed, 10 insertions(+), 7 deletions(-)
>>> Sorry for the delay in reviewing this, comments inline ...
>>>
>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>>>> index 7714d93..774f9ad 100644
>>>> --- a/kernel/auditfilter.c
>>>> +++ b/kernel/auditfilter.c
>>>> @@ -39,13 +39,13 @@
>>>>
>>>> * Locking model:
>>>> *
>>>>
>>>> * audit_filter_mutex:
>>>> - * Synchronizes writes and blocking reads of audit's filterlist
>>>> - * data. Rcu is used to traverse the filterlist and access
>>>> - * contents of structs audit_entry, audit_watch and opaque
>>>> - * LSM rules during filtering. If modified, these structures
>>>> - * must be copied and replace their counterparts in the filterlist.
>>>> - * An audit_parent struct is not accessed during filtering, so may
>>>> - * be written directly provided audit_filter_mutex is held.
>>>> + * Synchronizes writes and blocking reads of audit's filterlist
>>>> + * data. Rcu is used to traverse the filterlist and access
>>>> + * contents of structs audit_entry, audit_watch and opaque
>>>> + * LSM rules during filtering. If modified, these structures
>>>> + * must be copied and replace their counterparts in the filterlist.
>>>> + * An audit_parent struct is not accessed during filtering, so may
>>>> + * be written directly provided audit_filter_mutex is held.
>>>>
>>>> */
>>> Okay, that's fine.
> ...
>
>> As you might have guessed, this is one of my first patches. I wasn't sure if
>> a patch like this would even get reviewed, and responded to. I'm subscribed
>> to the linux-kernel mail group, and seeing what is acceptable.
>>
>> Thanks for the review. I don't plan on making a habit of submitting such
>> incredibly trivial patches, but you have to start somewhere, and I thought
>> it'd be hard to screw up by fixing a couple of trivial style errors.
> We all start somewhere, and with that in mind, if you want to resubmit this
> patch with only the fix above (fixing the whitespace in the comment block),
> I'll apply it. While the patch is trivial, it is does fix a minor nit with
> near-zero risk.
>
> I would encourage you to try something a bit more substantial next time, as
> they say, bug fixes are the quickest way to a maintainer's heart.
>
Thanks for the feedback. I'll resubmit. Now I get to figure out how to resubmit a patch with changes, so a good
learning experience for me. Pure Hobbyist at this time, but I love to learn.
Scott
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Scott Matheina @ 2015-10-22 0:30 UTC (permalink / raw)
To: Richard Guy Briggs, Joe Perches
Cc: Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <20151021153339.GA5269@madcap2.tricolour.ca>
On 10/21/2015 10:33 AM, Richard Guy Briggs wrote:
> On 15/10/21, Joe Perches wrote:
>> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
>>> On 15/10/18, Scott Matheina wrote:
>>>> On 10/14/2015 04:54 PM, Paul Moore wrote:
>>>>> On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
>> []
>>>>>> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
>> []
>>>>>> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
>>>>>> {
>>>>>> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
>>>>>> audit_free_rule(e);
>>>>>> +
>>>>>> }
>>>>> Why?
>>>> I was following the error messages in checkpatch.pl, but the warning
>>>> went away after adding this line. No problem with the code.
>>> That sounds like a bug in checkpatch.pl, since that blank line should be
>>> tween the declaration and the function call.
>> checkpatch message asks for a blank line after the
>> "struct audit_entry *e = ..." declaration.
> Well then maybe it is a bug in his interpretation of the output of
> checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those
> spaces? Did it pass?
The error did go away.
>>>>>> while (*list != ~0U) {
>>>>>> +
>>>>>> unsigned n = *list++;
>>>>>> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
>>>>>> kfree(p);
>>>>> Why?
>>>> This is the same as above. Just going through the checkpatch.pl
>>>> script, and looking for warnings to fix.
>>> Again, another manifestation of that bug? That blank line should be
>>> after the declaration and before the if statement.
>> []
>>> Well, I agree, you have to start somewhere... Too bad you hit a bug in
>>> checkpatch.pl!
>> Here too, not a bug in checkpatch.
>>
>> checkpatch output asks for a blank line after the
>> "unsigned n" declaration, not before.
> - RGB
>
> --
> Richard Guy Briggs <rbriggs@redhat.com>
> Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Paul Moore @ 2015-10-21 18:23 UTC (permalink / raw)
To: Scott Matheina; +Cc: eparis, linux-audit, linux-kernel, trivial
In-Reply-To: <5623DBF5.8020500@matheina.com>
On Sunday, October 18, 2015 12:50:45 PM Scott Matheina wrote:
> On 10/14/2015 04:54 PM, Paul Moore wrote:
> > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> >> Signed-off-by: Scott Matheina <scott@matheina.com>
> >> ---
> >>
> >> kernel/auditfilter.c | 17 ++++++++++-------
> >> 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > Sorry for the delay in reviewing this, comments inline ...
> >
> >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> >> index 7714d93..774f9ad 100644
> >> --- a/kernel/auditfilter.c
> >> +++ b/kernel/auditfilter.c
> >> @@ -39,13 +39,13 @@
> >>
> >> * Locking model:
> >> *
> >>
> >> * audit_filter_mutex:
> >> - * Synchronizes writes and blocking reads of audit's filterlist
> >> - * data. Rcu is used to traverse the filterlist and access
> >> - * contents of structs audit_entry, audit_watch and opaque
> >> - * LSM rules during filtering. If modified, these structures
> >> - * must be copied and replace their counterparts in the filterlist.
> >> - * An audit_parent struct is not accessed during filtering, so may
> >> - * be written directly provided audit_filter_mutex is held.
> >> + * Synchronizes writes and blocking reads of audit's filterlist
> >> + * data. Rcu is used to traverse the filterlist and access
> >> + * contents of structs audit_entry, audit_watch and opaque
> >> + * LSM rules during filtering. If modified, these structures
> >> + * must be copied and replace their counterparts in the filterlist.
> >> + * An audit_parent struct is not accessed during filtering, so may
> >> + * be written directly provided audit_filter_mutex is held.
> >>
> >> */
> >
> > Okay, that's fine.
...
> As you might have guessed, this is one of my first patches. I wasn't sure if
> a patch like this would even get reviewed, and responded to. I'm subscribed
> to the linux-kernel mail group, and seeing what is acceptable.
>
> Thanks for the review. I don't plan on making a habit of submitting such
> incredibly trivial patches, but you have to start somewhere, and I thought
> it'd be hard to screw up by fixing a couple of trivial style errors.
We all start somewhere, and with that in mind, if you want to resubmit this
patch with only the fix above (fixing the whitespace in the comment block),
I'll apply it. While the patch is trivial, it is does fix a minor nit with
near-zero risk.
I would encourage you to try something a bit more substantial next time, as
they say, bug fixes are the quickest way to a maintainer's heart.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Richard Guy Briggs @ 2015-10-21 15:33 UTC (permalink / raw)
To: Joe Perches
Cc: Scott Matheina, Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <1445435918.9220.9.camel@perches.com>
On 15/10/21, Joe Perches wrote:
> On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> > On 15/10/18, Scott Matheina wrote:
> > > On 10/14/2015 04:54 PM, Paul Moore wrote:
> > > > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
> []
> > > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> []
> > > >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> > > >> {
> > > >> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> > > >> audit_free_rule(e);
> > > >> +
> > > >> }
> > > > Why?
> > >
> > > I was following the error messages in checkpatch.pl, but the warning
> > > went away after adding this line. No problem with the code.
> >
> > That sounds like a bug in checkpatch.pl, since that blank line should be
> > tween the declaration and the function call.
>
> checkpatch message asks for a blank line after the
> "struct audit_entry *e = ..." declaration.
Well then maybe it is a bug in his interpretation of the output of
checkpatch.pl? Scott, did you re-run checkpatch.pl after adding those
spaces? Did it pass?
> > > >> while (*list != ~0U) {
> > > >> +
> > > >> unsigned n = *list++;
> > > >> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> > > >> kfree(p);
> > > > Why?
> > >
> > > This is the same as above. Just going through the checkpatch.pl
> > > script, and looking for warnings to fix.
> >
> > Again, another manifestation of that bug? That blank line should be
> > after the declaration and before the if statement.
> []
> > Well, I agree, you have to start somewhere... Too bad you hit a bug in
> > checkpatch.pl!
>
> Here too, not a bug in checkpatch.
>
> checkpatch output asks for a blank line after the
> "unsigned n" declaration, not before.
- RGB
--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545
^ permalink raw reply
* Re: [PATCH 2/2] Fixed Trivial Warnings in file: Deleted Spaces prior to tabs, and added lines. modified: kernel/auditfilter.c
From: Joe Perches @ 2015-10-21 13:58 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Scott Matheina, Paul Moore, linux-audit, trivial, linux-kernel
In-Reply-To: <20151019161035.GB11501@madcap2.tricolour.ca>
On Mon, 2015-10-19 at 12:10 -0400, Richard Guy Briggs wrote:
> On 15/10/18, Scott Matheina wrote:
> > On 10/14/2015 04:54 PM, Paul Moore wrote:
> > > On Saturday, October 10, 2015 08:57:55 PM Scott Matheina wrote:
[]
> > >> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
[]
> > >> @@ -109,6 +109,7 @@ void audit_free_rule_rcu(struct rcu_head *head)
> > >> {
> > >> struct audit_entry *e = container_of(head, struct audit_entry, rcu);
> > >> audit_free_rule(e);
> > >> +
> > >> }
> > > Why?
> >
> > I was following the error messages in checkpatch.pl, but the warning
> > went away after adding this line. No problem with the code.
>
> That sounds like a bug in checkpatch.pl, since that blank line should be
> tween the declaration and the function call.
checkpatch message asks for a blank line after the
"struct audit_entry *e = ..." declaration.
> > >> while (*list != ~0U) {
> > >> +
> > >> unsigned n = *list++;
> > >> if (n >= AUDIT_BITMASK_SIZE * 32 - AUDIT_SYSCALL_CLASSES) {
> > >> kfree(p);
> > > Why?
> >
> > This is the same as above. Just going through the checkpatch.pl
> > script, and looking for warnings to fix.
>
> Again, another manifestation of that bug? That blank line should be
> after the declaration and before the if statement.
[]
> Well, I agree, you have to start somewhere... Too bad you hit a bug in
> checkpatch.pl!
Here too, not a bug in checkpatch.
checkpatch output asks for a blank line after the
"unsigned n" declaration, not before.
^ permalink raw reply
* Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus
From: Stephen Smalley @ 2015-10-20 20:41 UTC (permalink / raw)
To: Paul Moore; +Cc: Paul Osmialowski, linux-security-module, linux-audit, selinux
In-Reply-To: <2112989.AEUouYpuBm@sifl>
On Mon, Oct 19, 2015 at 6:29 PM, Paul Moore <pmoore@redhat.com> wrote:
> On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
>> On 10/07/2015 07:08 PM, Paul Moore wrote:
>> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
>> > index ef63d65..1cb87b3 100644
>> > --- a/ipc/kdbus/connection.c
>> > +++ b/ipc/kdbus/connection.c
>> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
>> > kdbus_ep *ep,>
>> > if (!owner && (creds || pids || seclabel))
>> >
>> > return ERR_PTR(-EPERM);
>> >
>> > + ret = security_kdbus_conn_new(get_cred(file->f_cred),
>>
>> You only need to use get_cred() if saving a reference; otherwise, you'll
>> leak one here.
>
> Yes, that was a typo on my part, thanks.
>
>> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
>> what is used by their own checks; the former is typically the same as
>> current cred IIUC). For that matter, what about ep->node.creds vs ep->bus-
>> node.creds vs. ep->bus->domain->node.creds? Can they differ? Do we care?
>
> We don't want file->f_cred, per our previous discussions. I was working on
> this patchset in small chunks and while I added credential storing in the
> nodes, I forgot to update the hooks before I hit send, my apologies.
>
> My current thinking is to pass both the endpoint and bus credentials, as I
> believe they can differ. Both the bus and the endpoint inherit their security
> labels from their creator and while I don't have any specifics, I think it is
> reasonable to imagine those two processes having different security labels.
> Assuming we pass both credentials down to the LSM, I'm currently thinking of
> the following SELinux access controls for this hook:
>
> allow <current> bus_t:kdbus { connect };
> allow <current> ep_t:kdbus { use privileged activator monitor policy };
I think it would be simpler to apply an associate check when the
endpoint is created between the endpoint label and the bus label
(which will typically be the same), and then only check based on
endpoint label for all subsequent permission checks involving that
endpoint. Then you don't have to worry about which label to use for
all the other permission checks. And you get finer-grained control -
per-endpoint rather than only per-bus. In the common case, the labels
will be the same and it won't matter, but if you want to support some
MAC form of their 'custom endpoints' with further restrictions, you
can do that by labeling the endpoints uniquely and using those labels
in your permission checks.
Otherwise, I have to ask whether you need two different classes above
(bus vs endpoint), and whether the privileged/activator/monitor/policy
permissions properly belong with the endpoint label or the bus label.
At least some of those are bus-wide properties, not endpoint-specific,
but that's ok as long as we have established a relationship between
the endpoint label and bus label.
> ... besides the additional label, I added the kdbus:use permission and dropped
> the kdbus:owner permission. Considering that the endpoint label, ep_t, in the
> examples above, could be different from the current process, it seemed
> reasonable to want to control that interaction and I felt the fd:use
> permission was the closest existing control so I reused the permission name.
> I decided to drop the "owner" permission as it really wasn't the useful for
> anything anymore, it simply indicates that the current task is the DAC owner
> of the endpoint.
Can you 'use' an endpoint in any way other than to connect via it?
If not, I'd just call that connect (won't conflict if you get rid of
the separate bus check above), or distinguish it via separate classes
or as connectthrough vs connectto.
conn->owner is used to determine whether the caller can fake
credentials, skip kdbus policy checking, create an activator, monitor,
or policy holder connection, etc. Our options are:
1. Apply a SELinux check when it is set to see if the caller is
allowed to own the bus based on MAC labels and policy, and if not,
refuse to create the connection (that's what checking the owner
permission was doing).
2. Separately apply MAC checks over each of those abilities (fake
creds, override policy, create an activator, monitor, or policy
holder, etc) when there is an attempt to exercise them (not all during
connection creation), and selectively deny that ability. More
invasive, more potential for breakage for applications that don't
expect failure if they could create the connection in the first place.
3. Treat faking of DAC credentials and skipping of kdbus policy
checking as not of interest to MAC, leaving it only controlled by the
existing uid match or CAP_IPC_OWNER check. Simple, but seems to lose
some of the potential benefit of using SELinux for confining processes
using kdbus.
privileged actually seems less interesting than owner these days,
although I haven't done a thorough analysis.
IIUC, creating a monitor or policy holder connection has bus-wide
implications and we ought to control it so we can limit it to specific
processes, not just all uid-0 processes for the system bus and not
just all uid-<user> processes for the per-user bus. I'm not actually
clear on the implications of activator connections or why they are
limited in the same way.
>
>> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
>> > *conn,>
>> > return false;
>> >
>> > }
>> >
>> > - if (conn->owner)
>> > - return true;
>> > + if (!conn->owner &&
>> > + kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
>> > + hash) < KDBUS_POLICY_OWN)
>> > + return false;
>> >
>> > - res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
>> > - name, hash);
>> > - return res >= KDBUS_POLICY_OWN;
>> > + return (security_kdbus_own_name(conn_creds, name) == 0);
>>
>> Similar question here. conn_creds is the credentials of the creator of
>> the connection, typically the client/sender, right?
>> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
>> we want to ask "Can I own this name on this bus?".
>
> Yes, I think so.
>
> From a SELinux point of view I imagine we would want access controls along the
> lines of the following:
>
> allow current name_t:kdbus { own_name };
> allow current bus_t:kdbus { own_name };
>
> ... do we want to use different permissions? I doubt it would matter much
> either way.
If we keep them both, then they need separate classes or separate
permissions; I can't think of another case where we reuse the same
class and permission for two different objects.
Not clear that this kind of pairwise checking will work well. For
example, I should be able to own any name I like on my own bus, but
not on the system bus.
>> Note that their policy checks are based on conn->ep->policy_db, i.e. the
>> policy associated with the endpoint, and conn->owner is only true if the
>> connection creator has the same uid as the bus.
>
> I don't think this is significant for us.
>
>> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
>> > *conn,>
>> > to, KDBUS_POLICY_TALK))
>> >
>> > return false;
>> >
>> > - if (conn->owner)
>> > - return true;
>> > - if (uid_eq(conn_creds->euid, to->cred->uid))
>> > - return true;
>> > + if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
>> > + !kdbus_conn_policy_query_all(conn, conn_creds,
>> > + &conn->ep->bus->policy_db, to,
>> > + KDBUS_POLICY_TALK))
>> > + return false;
>> >
>> > - return kdbus_conn_policy_query_all(conn, conn_creds,
>> > - &conn->ep->bus->policy_db, to,
>> > - KDBUS_POLICY_TALK);
>> > + return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
>>
>> Here at least we have a notion of client and peer. But we still aren't
>> considering conn->ep or conn->ep->bus, whereas they are querying both
>> policy dbs for their decision. The parallel would be checking access to
>> the labels of both I suppose, unless we institute a control up front
>> over the relationship between the label of the endpoint and the label of
>> the bus.
>
> While accidental, as I forgot to update this patch as mentioned previously, I
> think the differences between kdbus DAC and kdbus MAC is okay. At this point
> we've already authorized the individual nodes to connect to the bus and now we
> are authorizing them to talk to each other; at this control point, it is the
> interaction between the two nodes that we care about, I don't think we care
> about what bus they use, do we?
Perhaps not.
>
>> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
>> > kdbus_conn *conn,>
>> > const struct cred *conn_creds,
>> > const char *name)
>> >
>> > {
>> >
>> > - int res;
>> > + if (!conn_creds)
>> > + conn_creds = conn->cred;
>> >
>> > /*
>> >
>> > * By default, all names are visible on a bus. SEE policies can only be
>> > * installed on custom endpoints, where by default no name is visible.
>> > */
>> >
>> > - if (!conn->ep->user)
>> > - return true;
>> > + if (conn->ep->user &&
>> > + kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
>> > + kdbus_strhash(name)) < KDBUS_POLICY_SEE)
>> > + return false;
>> >
>> > - res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
>> > - conn_creds ? : conn->cred,
>> > - name, kdbus_strhash(name));
>> > - return res >= KDBUS_POLICY_SEE;
>> > + return (security_kdbus_conn_see_name(conn_creds, name) == 0);
>>
>> Here they only define policy based on endpoints, not bus. Not sure what
>> we want, but we need at least one of their creds. Same for the rest.
>
> Once again, I'm not sure we care about the underlying bus at this point, do
> we? We've already authorized both the service and the client to connect to
> the bus, now I think all we care about is can the client see the service name.
> Do we really care about the label of the bus here?
Well, here we again have the issue that we should be able to see all
names on our own bus, but not necessarily on the system bus.
^ permalink raw reply
* Re: [RFC PATCH v3 2/5] lsm: introduce hooks for kdbus
From: Paul Moore @ 2015-10-19 22:29 UTC (permalink / raw)
To: Stephen Smalley
Cc: Paul Osmialowski, linux-security-module, linux-audit, selinux
In-Reply-To: <5617D58C.2070402@tycho.nsa.gov>
On Friday, October 09, 2015 10:56:12 AM Stephen Smalley wrote:
> On 10/07/2015 07:08 PM, Paul Moore wrote:
> > diff --git a/ipc/kdbus/connection.c b/ipc/kdbus/connection.c
> > index ef63d65..1cb87b3 100644
> > --- a/ipc/kdbus/connection.c
> > +++ b/ipc/kdbus/connection.c
> > @@ -108,6 +109,14 @@ static struct kdbus_conn *kdbus_conn_new(struct
> > kdbus_ep *ep,>
> > if (!owner && (creds || pids || seclabel))
> >
> > return ERR_PTR(-EPERM);
> >
> > + ret = security_kdbus_conn_new(get_cred(file->f_cred),
>
> You only need to use get_cred() if saving a reference; otherwise, you'll
> leak one here.
Yes, that was a typo on my part, thanks.
> Also, do we want file->f_cred here or ep->bus->node.creds (the latter is
> what is used by their own checks; the former is typically the same as
> current cred IIUC). For that matter, what about ep->node.creds vs ep->bus-
> node.creds vs. ep->bus->domain->node.creds? Can they differ? Do we care?
We don't want file->f_cred, per our previous discussions. I was working on
this patchset in small chunks and while I added credential storing in the
nodes, I forgot to update the hooks before I hit send, my apologies.
My current thinking is to pass both the endpoint and bus credentials, as I
believe they can differ. Both the bus and the endpoint inherit their security
labels from their creator and while I don't have any specifics, I think it is
reasonable to imagine those two processes having different security labels.
Assuming we pass both credentials down to the LSM, I'm currently thinking of
the following SELinux access controls for this hook:
allow <current> bus_t:kdbus { connect };
allow <current> ep_t:kdbus { use privileged activator monitor policy };
... besides the additional label, I added the kdbus:use permission and dropped
the kdbus:owner permission. Considering that the endpoint label, ep_t, in the
examples above, could be different from the current process, it seemed
reasonable to want to control that interaction and I felt the fd:use
permission was the closest existing control so I reused the permission name.
I decided to drop the "owner" permission as it really wasn't the useful for
anything anymore, it simply indicates that the current task is the DAC owner
of the endpoint.
> > @@ -1435,12 +1444,12 @@ bool kdbus_conn_policy_own_name(struct kdbus_conn
> > *conn,>
> > return false;
> >
> > }
> >
> > - if (conn->owner)
> > - return true;
> > + if (!conn->owner &&
> > + kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds, name,
> > + hash) < KDBUS_POLICY_OWN)
> > + return false;
> >
> > - res = kdbus_policy_query(&conn->ep->bus->policy_db, conn_creds,
> > - name, hash);
> > - return res >= KDBUS_POLICY_OWN;
> > + return (security_kdbus_own_name(conn_creds, name) == 0);
>
> Similar question here. conn_creds is the credentials of the creator of
> the connection, typically the client/sender, right?
> conn->ep->bus->node.creds are the credentials of the bus owner, so don't
> we want to ask "Can I own this name on this bus?".
Yes, I think so.
>From a SELinux point of view I imagine we would want access controls along the
lines of the following:
allow current name_t:kdbus { own_name };
allow current bus_t:kdbus { own_name };
... do we want to use different permissions? I doubt it would matter much
either way.
> Note that their policy checks are based on conn->ep->policy_db, i.e. the
> policy associated with the endpoint, and conn->owner is only true if the
> connection creator has the same uid as the bus.
I don't think this is significant for us.
> > @@ -1465,14 +1474,13 @@ bool kdbus_conn_policy_talk(struct kdbus_conn
> > *conn,>
> > to, KDBUS_POLICY_TALK))
> >
> > return false;
> >
> > - if (conn->owner)
> > - return true;
> > - if (uid_eq(conn_creds->euid, to->cred->uid))
> > - return true;
> > + if (!conn->owner && !uid_eq(conn_creds->euid, to->cred->uid) &&
> > + !kdbus_conn_policy_query_all(conn, conn_creds,
> > + &conn->ep->bus->policy_db, to,
> > + KDBUS_POLICY_TALK))
> > + return false;
> >
> > - return kdbus_conn_policy_query_all(conn, conn_creds,
> > - &conn->ep->bus->policy_db, to,
> > - KDBUS_POLICY_TALK);
> > + return (security_kdbus_conn_talk(conn_creds, to->cred) == 0);
>
> Here at least we have a notion of client and peer. But we still aren't
> considering conn->ep or conn->ep->bus, whereas they are querying both
> policy dbs for their decision. The parallel would be checking access to
> the labels of both I suppose, unless we institute a control up front
> over the relationship between the label of the endpoint and the label of
> the bus.
While accidental, as I forgot to update this patch as mentioned previously, I
think the differences between kdbus DAC and kdbus MAC is okay. At this point
we've already authorized the individual nodes to connect to the bus and now we
are authorizing them to talk to each other; at this control point, it is the
interaction between the two nodes that we care about, I don't think we care
about what bus they use, do we?
> > @@ -1491,19 +1499,19 @@ bool kdbus_conn_policy_see_name_unlocked(struct
> > kdbus_conn *conn,>
> > const struct cred *conn_creds,
> > const char *name)
> >
> > {
> >
> > - int res;
> > + if (!conn_creds)
> > + conn_creds = conn->cred;
> >
> > /*
> >
> > * By default, all names are visible on a bus. SEE policies can only be
> > * installed on custom endpoints, where by default no name is visible.
> > */
> >
> > - if (!conn->ep->user)
> > - return true;
> > + if (conn->ep->user &&
> > + kdbus_policy_query_unlocked(&conn->ep->policy_db, conn_creds, name,
> > + kdbus_strhash(name)) < KDBUS_POLICY_SEE)
> > + return false;
> >
> > - res = kdbus_policy_query_unlocked(&conn->ep->policy_db,
> > - conn_creds ? : conn->cred,
> > - name, kdbus_strhash(name));
> > - return res >= KDBUS_POLICY_SEE;
> > + return (security_kdbus_conn_see_name(conn_creds, name) == 0);
>
> Here they only define policy based on endpoints, not bus. Not sure what
> we want, but we need at least one of their creds. Same for the rest.
Once again, I'm not sure we care about the underlying bus at this point, do
we? We've already authorized both the service and the client to connect to
the bus, now I think all we care about is can the client see the service name.
Do we really care about the label of the bus here?
> > @@ -1530,10 +1541,13 @@ static bool kdbus_conn_policy_see(struct
> > kdbus_conn *conn,>
> > * peers from each other, unless you see at least _one_ name of the
> > * peer.
> > */
> >
> > - return !conn->ep->user ||
> > - kdbus_conn_policy_query_all(conn, conn_creds,
> > - &conn->ep->policy_db, whom,
> > - KDBUS_POLICY_SEE);
> > + if (conn->ep->user &&
> > + !kdbus_conn_policy_query_all(conn, conn_creds,
> > + &conn->ep->policy_db, whom,
> > + KDBUS_POLICY_SEE))
> > + return false;
> > +
> > + return (security_kdbus_conn_see(conn_creds, whom->cred) == 0);
> >
> > }
Very similar to the kdbus_conn_talk hook above, the credentials above still
look reasonable to me.
> > @@ -1567,18 +1584,22 @@ bool kdbus_conn_policy_see_notification(struct
> > kdbus_conn *conn,>
> > case KDBUS_ITEM_NAME_ADD:
> > case KDBUS_ITEM_NAME_REMOVE:
> >
> > case KDBUS_ITEM_NAME_CHANGE:
> > - return kdbus_conn_policy_see_name(conn, conn_creds,
> > - msg->items[0].name_change.name);
> > + if (!kdbus_conn_policy_see_name(conn, conn_creds,
> > + msg->items[0].name_change.name))
> > + return false;
> >
> > case KDBUS_ITEM_ID_ADD:
> >
> > case KDBUS_ITEM_ID_REMOVE:
> > - return true;
> > + /* fall through for the LSM check */
> > + break;
> >
> > default:
> > WARN(1, "Invalid type for notification broadcast: %llu\n",
> >
> > (unsigned long long)msg->items[0].type);
> >
> > return false;
> >
> > }
> >
> > +
> > + return (security_kdbus_conn_see_notification(conn_creds) == 0);
> >
> > }
This also seems okay.
> > diff --git a/ipc/kdbus/fs.c b/ipc/kdbus/fs.c
> > index 68818a8..4e84e89 100644
> > --- a/ipc/kdbus/fs.c
> > +++ b/ipc/kdbus/fs.c
> > @@ -200,6 +202,10 @@ static struct inode *fs_inode_get(struct super_block
> > *sb,>
> > if (!(inode->i_state & I_NEW))
> >
> > return inode;
> >
> > + ret = security_kdbus_init_inode(inode, node->creds);
> > + if (ret)
> > + return ERR_PTR(ret);
>
> Need to put the inode.
Thanks. It looks like I need to make a call to iget_failed() before
returning.
--
paul moore
security @ redhat
^ 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