LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid
@ 2008-03-18  9:24 Pavel Emelyanov
  2008-03-18 23:29 ` Eric Paris
  0 siblings, 1 reply; 5+ messages in thread
From: Pavel Emelyanov @ 2008-03-18  9:24 UTC (permalink / raw)
  To: Eric Paris, linux-audit; +Cc: Thomas Graf, Linux Kernel Mailing List

This patch is based on the one from Thomas.

The kauditd_thread() calls the netlink_unicast() and passes 
the audit_pid to it. The audit_pid, in turn, is received from 
the user space and the tool (I've checked the audit v1.6.9) 
uses getpid() to pass one in the kernel. Besides, this tool 
doesn't bind the netlink socket to this id, but simply creates 
it allowing the kernel to auto-bind one.

That's the preamble.

The problem is that netlink_autobind() _does_not_ guarantees
that the socket will be auto-bound to the current pid. Instead
it uses the current pid as a hint to start looking for a free
id. So, in case of conflict, the audit messages can be sent
to a wrong socket. This can happen (it's unlikely, but can be)
in case some task opens more than one netlink sockets and then
the audit one starts - in this case the audit's pid can be busy
and its socket will be bound to another id.

The proposal is to introduce an audit_nlk_pid in audit subsys,
that will point to the netlink socket to send packets to. It
will most often be equal to audit_pid. The socket id can be 
got from the skb's netlink CB right in the audit_receive_msg.

Later, if the audit tools will bind the socket themselves, the
kernel will have to provide a way to setup the audit_nlk_pid
as well.

A good side effect of this patch is that audit_pid can later 
be converted to struct pid, as it is not longer safe to use 
pid_t-s in the presence of pid namespaces. But audit code still 
uses the tgid from task_struct in the audit_signal_info and in
the audit_filter_syscall.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

---

diff --git a/kernel/audit.c b/kernel/audit.c
index 10c4930..40b3a34 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -78,8 +78,10 @@ static int	audit_default;
 /* If auditing cannot proceed, audit_failure selects what happens. */
 static int	audit_failure = AUDIT_FAIL_PRINTK;
 
-/* If audit records are to be written to the netlink socket, audit_pid
+/* If audit records are to be written to the netlink socket, audit_nlk_pid
  * contains the (non-zero) pid. */
+static int	audit_nlk_pid;
+
 int		audit_pid;
 
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
@@ -349,13 +351,13 @@ static int kauditd_thread(void *dummy)
 		skb = skb_dequeue(&audit_skb_queue);
 		wake_up(&audit_backlog_wait);
 		if (skb) {
-			if (audit_pid) {
-				int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
+			if (audit_nlk_pid) {
+				int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
 				if (err < 0) {
 					BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
-					printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
+					printk(KERN_ERR "audit: *NO* daemon at audit_nlk_pid=%d\n", audit_nlk_pid);
 					audit_log_lost("auditd dissapeared\n");
-					audit_pid = 0;
+					audit_nlk_pid = 0;
 				}
 			} else {
 				if (printk_ratelimit())
@@ -626,6 +628,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 							sid, 1);
 
 			audit_pid = new_pid;
+			audit_nlk_pid = NETLINK_CB(skb).pid;
 		}
 		if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
 			err = audit_set_rate_limit(status_get->rate_limit,
@@ -1354,7 +1357,7 @@ void audit_log_end(struct audit_buffer *ab)
 		audit_log_lost("rate limit exceeded");
 	} else {
 		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
-		if (audit_pid) {
+		if (audit_nlk_pid) {
 			nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0);
 			skb_queue_tail(&audit_skb_queue, ab->skb);
 			ab->skb = NULL;

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid
  2008-03-18  9:24 [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid Pavel Emelyanov
@ 2008-03-18 23:29 ` Eric Paris
  2008-03-18 23:35   ` Eric Paris
  2008-03-19  9:11   ` Pavel Emelyanov
  0 siblings, 2 replies; 5+ messages in thread
From: Eric Paris @ 2008-03-18 23:29 UTC (permalink / raw)
  To: Pavel Emelyanov
  Cc: linux-audit, Thomas Graf, Linux Kernel Mailing List, aviro, sgrubb, akpm

On 3/18/08, Pavel Emelyanov <xemul@openvz.org> wrote:

Can we please just leave audit_pid as the one flag to tell us if
auditd is supposedly up there in userspace?  Use audit_nlk_pid to send
messages to the right place, but leave everything else to do with
audit_pid alone.


>  diff --git a/kernel/audit.c b/kernel/audit.c
>  index 10c4930..40b3a34 100644
>  --- a/kernel/audit.c
>  +++ b/kernel/audit.c
>  @@ -78,8 +78,10 @@ static int   audit_default;
>   /* If auditing cannot proceed, audit_failure selects what happens. */
>   static int     audit_failure = AUDIT_FAIL_PRINTK;
>
>  -/* If audit records are to be written to the netlink socket, audit_pid
>  +/* If audit records are to be written to the netlink socket, audit_nlk_pid

/* If audit records are to be written to the netlink socket, audit_pid
contain the pid of the auditd process and audit_nlk_pid will contain
the pid to use to send netlink messages to that process. */

>   * contains the (non-zero) pid. */
>  +static int     audit_nlk_pid;
>  +
>   int            audit_pid;
>
>   /* If audit_rate_limit is non-zero, limit the rate of sending audit records
>  @@ -349,13 +351,13 @@ static int kauditd_thread(void *dummy)
>                 skb = skb_dequeue(&audit_skb_queue);
>                 wake_up(&audit_backlog_wait);
>                 if (skb) {
>  -                       if (audit_pid) {
>  -                               int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
>  +                       if (audit_nlk_pid) {

unneeded change

>  +                               int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);

good.

>                                 if (err < 0) {
>                                         BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
>  -                                       printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
>  +                                       printk(KERN_ERR "audit: *NO* daemon at audit_nlk_pid=%d\n", audit_nlk_pid);

userspace is never going to care about audit_nlk_pid, its meaningless
at that level.  This message should continue to use audit_pid.

>                                         audit_log_lost("auditd dissapeared\n");
>  -                                       audit_pid = 0;

We still need to 0 audit_pid otherwise all of the tests which use it
are bogus (aka things like audit_panic and audit_filter_syscall you
mentioned)

>  +                                       audit_nlk_pid = 0;
>                                 }
>                         } else {
>                                 if (printk_ratelimit())
>  @@ -626,6 +628,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>                                                         sid, 1);
>
>                         audit_pid = new_pid;
>  +                       audit_nlk_pid = NETLINK_CB(skb).pid;
>                 }
>                 if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
>                         err = audit_set_rate_limit(status_get->rate_limit,
>  @@ -1354,7 +1357,7 @@ void audit_log_end(struct audit_buffer *ab)
>                 audit_log_lost("rate limit exceeded");
>         } else {
>                 struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
>  -               if (audit_pid) {
>  +               if (audit_nlk_pid) {

don't need this if we zero audit_pid.

>                         nlh->nlmsg_len = ab->skb->len - NLMSG_SPACE(0);
>                         skb_queue_tail(&audit_skb_queue, ab->skb);
>                         ab->skb = NULL;
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid
  2008-03-18 23:29 ` Eric Paris
@ 2008-03-18 23:35   ` Eric Paris
  2008-03-19  9:11   ` Pavel Emelyanov
  1 sibling, 0 replies; 5+ messages in thread
From: Eric Paris @ 2008-03-18 23:35 UTC (permalink / raw)
  To: Pavel Emelyanov
  Cc: linux-audit, Thomas Graf, Linux Kernel Mailing List, aviro, sgrubb, akpm

On 3/18/08, Eric Paris <eparis@parisplace.org> wrote:
> On 3/18/08, Pavel Emelyanov <xemul@openvz.org> wrote:

>  >  @@ -626,6 +628,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>  >                                                         sid, 1);
>  >
>  >                         audit_pid = new_pid;
>  >  +                       audit_nlk_pid = NETLINK_CB(skb).pid;
>  >                 }
>  >                 if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
>  >                         err = audit_set_rate_limit(status_get->rate_limit,

Shouldn't the above be:

if (audit_pid)
    audit_nlk_pid = NETLINK_CB(skb).pid;
else
   audit_nlk_pid = 0;

otherwise I don't see how you can cleanly stop the userspace auditd.....

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid
  2008-03-18 23:29 ` Eric Paris
  2008-03-18 23:35   ` Eric Paris
@ 2008-03-19  9:11   ` Pavel Emelyanov
  2008-03-19 12:04     ` Eric Paris
  1 sibling, 1 reply; 5+ messages in thread
From: Pavel Emelyanov @ 2008-03-19  9:11 UTC (permalink / raw)
  To: Eric Paris
  Cc: linux-audit, Thomas Graf, Linux Kernel Mailing List, aviro, sgrubb, akpm

Eric Paris wrote:
> On 3/18/08, Pavel Emelyanov <xemul@openvz.org> wrote:
> 
> Can we please just leave audit_pid as the one flag to tell us if
> auditd is supposedly up there in userspace?  Use audit_nlk_pid to send
> messages to the right place, but leave everything else to do with
> audit_pid alone.

OK, point taken ;) Is that patch looks OK? If yes, I'll comment
it properly and re-send to Andrew.

The audit_nlk_pid reset to 0 is not required, since all the
decisions are taken based on audit_pid value only.

Signed-off-by: Pavel Emelyanov <xemul@openvz.org>

diff --git a/kernel/audit.c b/kernel/audit.c
index ccc7d0e..12a0278 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -77,9 +77,13 @@ static int	audit_default;
 /* If auditing cannot proceed, audit_failure selects what happens. */
 static int	audit_failure = AUDIT_FAIL_PRINTK;
 
-/* If audit records are to be written to the netlink socket, audit_pid
- * contains the (non-zero) pid. */
+/*
+ * If audit records are to be written to the netlink socket, audit_pid
+ * contains the pid of the auditd process and audit_nlk_pid contains
+ * the pid to use to send netlink messages to that process.
+ */
 int		audit_pid;
+static int	audit_nlk_pid;
 
 /* If audit_rate_limit is non-zero, limit the rate of sending audit records
  * to that number per second.  This prevents DoS attacks, but results in
@@ -349,7 +353,7 @@ static int kauditd_thread(void *dummy)
 		wake_up(&audit_backlog_wait);
 		if (skb) {
 			if (audit_pid) {
-				int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
+				int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
 				if (err < 0) {
 					BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
 					printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
@@ -626,6 +630,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 							sid, 1);
 
 			audit_pid = new_pid;
+			audit_nlk_pid = NETLINK_CB(skb).pid;
 		}
 		if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
 			err = audit_set_rate_limit(status_get->rate_limit,

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid
  2008-03-19  9:11   ` Pavel Emelyanov
@ 2008-03-19 12:04     ` Eric Paris
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Paris @ 2008-03-19 12:04 UTC (permalink / raw)
  To: Pavel Emelyanov
  Cc: linux-audit, Thomas Graf, Linux Kernel Mailing List, aviro, sgrubb, akpm

On 3/19/08, Pavel Emelyanov <xemul@openvz.org> wrote:
> Eric Paris wrote:
>  > On 3/18/08, Pavel Emelyanov <xemul@openvz.org> wrote:
>  >
>  > Can we please just leave audit_pid as the one flag to tell us if
>  > auditd is supposedly up there in userspace?  Use audit_nlk_pid to send
>  > messages to the right place, but leave everything else to do with
>  > audit_pid alone.
>
>
> OK, point taken ;) Is that patch looks OK? If yes, I'll comment
>  it properly and re-send to Andrew.
>
>  The audit_nlk_pid reset to 0 is not required, since all the
>  decisions are taken based on audit_pid value only.

I looks good to me.  Feel free to add Acked-by: Eric Paris
<eparis@redhat.com> on your submission.

-Eric

>
>  Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
>
>
>  diff --git a/kernel/audit.c b/kernel/audit.c
>
> index ccc7d0e..12a0278 100644
>
> --- a/kernel/audit.c
>  +++ b/kernel/audit.c
>
> @@ -77,9 +77,13 @@ static int   audit_default;
>
>  /* If auditing cannot proceed, audit_failure selects what happens. */
>   static int     audit_failure = AUDIT_FAIL_PRINTK;
>
>  -/* If audit records are to be written to the netlink socket, audit_pid
>
> - * contains the (non-zero) pid. */
>  +/*
>  + * If audit records are to be written to the netlink socket, audit_pid
>  + * contains the pid of the auditd process and audit_nlk_pid contains
>  + * the pid to use to send netlink messages to that process.
>  + */
>   int            audit_pid;
>
> +static int     audit_nlk_pid;
>
>
>  /* If audit_rate_limit is non-zero, limit the rate of sending audit records
>
>   * to that number per second.  This prevents DoS attacks, but results in
>  @@ -349,7 +353,7 @@ static int kauditd_thread(void *dummy)
>
>                 wake_up(&audit_backlog_wait);
>                 if (skb) {
>                         if (audit_pid) {
>  -                               int err = netlink_unicast(audit_sock, skb, audit_pid, 0);
>
> +                               int err = netlink_unicast(audit_sock, skb, audit_nlk_pid, 0);
>
>                                 if (err < 0) {
>                                         BUG_ON(err != -ECONNREFUSED); /* Shoudn't happen */
>                                         printk(KERN_ERR "audit: *NO* daemon at audit_pid=%d\n", audit_pid);
>
> @@ -626,6 +630,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>
>                                                         sid, 1);
>
>                         audit_pid = new_pid;
>  +                       audit_nlk_pid = NETLINK_CB(skb).pid;
>                 }
>                 if (status_get->mask & AUDIT_STATUS_RATE_LIMIT)
>                         err = audit_set_rate_limit(status_get->rate_limit,
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2008-03-20  1:58 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-18  9:24 [PATCH] Audit: netlink socket can be auto-bound to pid other than current->pid Pavel Emelyanov
2008-03-18 23:29 ` Eric Paris
2008-03-18 23:35   ` Eric Paris
2008-03-19  9:11   ` Pavel Emelyanov
2008-03-19 12:04     ` Eric Paris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).