Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-13 16:17, Cong Wang wrote:
> On Tue, Dec 13, 2016 at 2:52 AM, Richard Guy Briggs  wrote:
> > It is actually the audit_pid and audit_nlk_portid that I care about
> > more.  The audit daemon could vanish or close the socket while the
> > kernel sock to which it was attached is still quite valid.  Accessing
> > the set of three atomically is the urge.  I wonder if it makes more
> > sense to test for the presence of auditd using audit_sock rather than
> > audit_pid, but still keep audit_pid for our reporting and replacement
> > strategy.  Another idea would be to put the three in one struct.
> 
> Note, the process has audit_pid should hold a refcnt to the netns too,
> so the netns can't be gone until that process is gone.

I noted that.  I did wonder if there might be a problem if all the
processes were moved to another netns with the struct sock stuck in the
now process-void netns.

This is alluded-to in 6f285b19d09f ("audit: Send replies in the proper
network namespace.").

> > Can someone explain how they think the original test was able to trigger
> > this GPF?  Network namespace shutdown while something pretended to set
> > up a new auditd?  That's impressive for a fuzzer if that's the case...
> > Is there an strace?  I guess it is all in test().
> 
> I am surprised you still don't get the race condition even when you
> are now working on v2...
> 
> The race happens in this scenarios :
> 
> 1) Create a new netns
> 
> 2) In the new netns, communicate with kauditd to set audit_sock
> 
> 3) Generate some audit messages, so kauditd will keep sending them
> via audit_sock
> 
> 4) exit the netns
> 
> 5) the previous audit_sock is now going away, but kaudit_sock could still
> access it in this small window.

Ah ok that fits...

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Tue, Dec 13, 2016 at 2:52 AM, Richard Guy Briggs  wrote:
> It is actually the audit_pid and audit_nlk_portid that I care about
> more.  The audit daemon could vanish or close the socket while the
> kernel sock to which it was attached is still quite valid.  Accessing
> the set of three atomically is the urge.  I wonder if it makes more
> sense to test for the presence of auditd using audit_sock rather than
> audit_pid, but still keep audit_pid for our reporting and replacement
> strategy.  Another idea would be to put the three in one struct.

Note, the process has audit_pid should hold a refcnt to the netns too,
so the netns can't be gone until that process is gone.

>
> Can someone explain how they think the original test was able to trigger
> this GPF?  Network namespace shutdown while something pretended to set
> up a new auditd?  That's impressive for a fuzzer if that's the case...
> Is there an strace?  I guess it is all in test().
>

I am surprised you still don't get the race condition even when you
are now working on v2...

The race happens in this scenarios :

1) Create a new netns

2) In the new netns, communicate with kauditd to set audit_sock

3) Generate some audit messages, so kauditd will keep sending them
via audit_sock

4) exit the netns

5) the previous audit_sock is now going away, but kaudit_sock could still
access it in this small window.

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-12 16:10, Cong Wang wrote:
> On Mon, Dec 12, 2016 at 2:02 AM, Richard Guy Briggs  wrote:
> > On 2016-12-09 20:13, Cong Wang wrote:
> >> Netlink notifier can safely be converted to blocking one, I will send
> >> a patch.
> >
> > I had a quick look at how that might happen.  The netlink notifier chain
> > is atomic.  Would the registered callback funciton need to spawn a
> > one-time thread to avoid blocking?
> 
> It is already non-atomic now:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=efa172f42836477bf1ac3c9a3053140df764699c

Ok, that is recent...  It is still less attractive as you point out due
to the overhead, but still worth considering if we can't find another
way.

> > I had a look at your patch.  It looks attractively simple.  The audit
> > next tree has patches queued that add an audit_reset function that will
> > require more work.  I still see some potential gaps.
> >
> > - If the process messes up (or the sock lookup messes up) it is reset
> >   in the kauditd thread under the audit_cmd_mutex.
> >
> > - If the process exits normally or is replaced due to an audit_replace
> >   error, it is reset from audit_receive_skb under the audit_cmd_mutex.
> >
> > - If the process dies before the kauditd thread notices, either reap it
> >   via notifier callback or it needs a check on net exit to reset.  This
> >   last one appears necessary to decrement the sock refcount so the sock
> >   can be released in netlink_kernel_release().
> >
> > If we want to be proactive and use the netlink notifier, we assume the
> > overhead of adding to the netlink notifier chain and eliminate all the
> > other reset calls under the kauditd thread.  If we are ok being
> > reactionary, then we'll at least need the net exit check on audit_sock.
> 
> I don't see why we need to check it in net exit if we use refcnt,
> because we have two different users of audit_sock: kauditd and
> netns, if both take care of refcnt properly, we don't need to worry
> about who is the last, no matter what failures occur in what order.

It is actually the audit_pid and audit_nlk_portid that I care about
more.  The audit daemon could vanish or close the socket while the
kernel sock to which it was attached is still quite valid.  Accessing
the set of three atomically is the urge.  I wonder if it makes more
sense to test for the presence of auditd using audit_sock rather than
audit_pid, but still keep audit_pid for our reporting and replacement
strategy.  Another idea would be to put the three in one struct.

Can someone explain how they think the original test was able to trigger
this GPF?  Network namespace shutdown while something pretended to set
up a new auditd?  That's impressive for a fuzzer if that's the case...
Is there an strace?  I guess it is all in test().

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-13 02:51, Richard Guy Briggs wrote:
> On 2016-12-09 23:40, Cong Wang wrote:
> > On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang  wrote:
> > > On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs  
> > > wrote:
> > >> On 2016-12-08 22:57, Cong Wang wrote:
> > >>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  
> > >>> wrote:
> > >>> > I also tried to extend Cong Wang's idea to attempt to proactively 
> > >>> > respond to a
> > >>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a 
> > >>> > locking error
> > >>> > stack dump using mutex_lock(_cmd_mutex) in the notifier 
> > >>> > callback.
> > >>> > Eliminating the lock since the sock is dead anways eliminates the 
> > >>> > error.
> > >>> >
> > >>> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile 
> > >>> > I'll try to
> > >>> > get the test case to compile.
> > >>>
> > >>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
> > >>> 'audit_pid'
> > >>> are updated as a whole and race between audit_receive_msg() and
> > >>> NETLINK_URELEASE.
> > >>
> > >> This is what I expected and why I originally added the mutex lock in the
> > >> callback...  The dumps I got were bare with no wrapper identifying the
> > >> process context or specific error, so I'm at a bit of a loss how to
> > >> solve this (without thinking more about it) other than instinctively
> > >> removing the mutex.
> > >
> > > Netlink notifier can safely be converted to blocking one, I will send
> > > a patch.
> > >
> > > But I seriously doubt you really need NETLINK_URELEASE here,
> > > it adds nothing but overhead, b/c the netlink notifier is called on
> > > every netlink socket in the system, but for net exit path, that is
> > > relatively a slow path.
> > >
> > > Also, kauditd_send_skb() needs audit_cmd_mutex too.
> > 
> > Please let me know what you think about the attached patch?
> > 
> > Thanks!
> 
> > commit a12b43ee814625933ff155c20dc863c59cfcf240
> > Author: Cong Wang 
> > Date:   Fri Dec 9 17:56:42 2016 -0800
> > 
> > audit: close a race condition on audit_sock
> > 
> > Signed-off-by: Cong Wang 
> > 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f1ca116..ab947d8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb)
> > snprintf(s, sizeof(s), "audit_pid=%d reset", 
> > audit_pid);
> > audit_log_lost(s);
> > audit_pid = 0;
> > +   audit_nlk_portid = 0;
> > +   sock_put(audit_sock);
> > audit_sock = NULL;
> > } else {
> > pr_warn("re-scheduling(#%d) write to 
> > audit_pid=%d\n",
> > @@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, 
> > struct nlmsghdr *nlh)
> > audit_log_config_change("audit_pid", new_pid, 
> > audit_pid, 1);
> > audit_pid = new_pid;
> > audit_nlk_portid = NETLINK_CB(skb).portid;
> > +   sock_hold(skb->sk);
> > +   if (audit_sock)
> > +   sock_put(audit_sock);
> > audit_sock = skb->sk;
> > }
> > if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> > @@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net 
> > *net)
> >  {
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> > struct sock *sock = aunet->nlsk;
> > -   if (sock == audit_sock) {
> > -   audit_pid = 0;
> > -   audit_sock = NULL;
> > -   }
> 
> So how does this not leak memory leaving the sock refcount incremented
> by the registered audit daemon when that daemon shuts down normally?

Sorry, that should have been: How does it not leak if auditd exits
abnormally without sending a shutdown message, but no message is sent on
the queue to trigger an error before the net namespace exits?

> - RGB

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-09 23:40, Cong Wang wrote:
> On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang  wrote:
> > On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs  wrote:
> >> On 2016-12-08 22:57, Cong Wang wrote:
> >>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  
> >>> wrote:
> >>> > I also tried to extend Cong Wang's idea to attempt to proactively 
> >>> > respond to a
> >>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
> >>> > error
> >>> > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> >>> > Eliminating the lock since the sock is dead anways eliminates the error.
> >>> >
> >>> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll 
> >>> > try to
> >>> > get the test case to compile.
> >>>
> >>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
> >>> 'audit_pid'
> >>> are updated as a whole and race between audit_receive_msg() and
> >>> NETLINK_URELEASE.
> >>
> >> This is what I expected and why I originally added the mutex lock in the
> >> callback...  The dumps I got were bare with no wrapper identifying the
> >> process context or specific error, so I'm at a bit of a loss how to
> >> solve this (without thinking more about it) other than instinctively
> >> removing the mutex.
> >
> > Netlink notifier can safely be converted to blocking one, I will send
> > a patch.
> >
> > But I seriously doubt you really need NETLINK_URELEASE here,
> > it adds nothing but overhead, b/c the netlink notifier is called on
> > every netlink socket in the system, but for net exit path, that is
> > relatively a slow path.
> >
> > Also, kauditd_send_skb() needs audit_cmd_mutex too.
> 
> Please let me know what you think about the attached patch?
> 
> Thanks!

> commit a12b43ee814625933ff155c20dc863c59cfcf240
> Author: Cong Wang 
> Date:   Fri Dec 9 17:56:42 2016 -0800
> 
> audit: close a race condition on audit_sock
> 
> Signed-off-by: Cong Wang 
> 
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..ab947d8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb)
>   snprintf(s, sizeof(s), "audit_pid=%d reset", 
> audit_pid);
>   audit_log_lost(s);
>   audit_pid = 0;
> + audit_nlk_portid = 0;
> + sock_put(audit_sock);
>   audit_sock = NULL;
>   } else {
>   pr_warn("re-scheduling(#%d) write to 
> audit_pid=%d\n",
> @@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
> nlmsghdr *nlh)
>   audit_log_config_change("audit_pid", new_pid, 
> audit_pid, 1);
>   audit_pid = new_pid;
>   audit_nlk_portid = NETLINK_CB(skb).portid;
> + sock_hold(skb->sk);
> + if (audit_sock)
> + sock_put(audit_sock);
>   audit_sock = skb->sk;
>   }
>   if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
> @@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
>   struct audit_net *aunet = net_generic(net, audit_net_id);
>   struct sock *sock = aunet->nlsk;
> - if (sock == audit_sock) {
> - audit_pid = 0;
> - audit_sock = NULL;
> - }

So how does this not leak memory leaving the sock refcount incremented
by the registered audit daemon when that daemon shuts down normally?


- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Mon, Dec 12, 2016 at 2:02 AM, Richard Guy Briggs  wrote:
> On 2016-12-09 20:13, Cong Wang wrote:
>> Netlink notifier can safely be converted to blocking one, I will send
>> a patch.
>
> I had a quick look at how that might happen.  The netlink notifier chain
> is atomic.  Would the registered callback funciton need to spawn a
> one-time thread to avoid blocking?

It is already non-atomic now:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=efa172f42836477bf1ac3c9a3053140df764699c


> I had a look at your patch.  It looks attractively simple.  The audit
> next tree has patches queued that add an audit_reset function that will
> require more work.  I still see some potential gaps.
>
> - If the process messes up (or the sock lookup messes up) it is reset
>   in the kauditd thread under the audit_cmd_mutex.
>
> - If the process exits normally or is replaced due to an audit_replace
>   error, it is reset from audit_receive_skb under the audit_cmd_mutex.
>
> - If the process dies before the kauditd thread notices, either reap it
>   via notifier callback or it needs a check on net exit to reset.  This
>   last one appears necessary to decrement the sock refcount so the sock
>   can be released in netlink_kernel_release().
>
> If we want to be proactive and use the netlink notifier, we assume the
> overhead of adding to the netlink notifier chain and eliminate all the
> other reset calls under the kauditd thread.  If we are ok being
> reactionary, then we'll at least need the net exit check on audit_sock.
>

I don't see why we need to check it in net exit if we use refcnt,
because we have two different users of audit_sock: kauditd and
netns, if both take care of refcnt properly, we don't need to worry
about who is the last, no matter what failures occur in what order.

Re: netlink: GPF in sock_sndtimeo

[Dmitry Vyukov]

On Sat, Dec 10, 2016 at 8:40 AM, Cong Wang  wrote:
>>> On 2016-12-08 22:57, Cong Wang wrote:
 On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  
 wrote:
 > I also tried to extend Cong Wang's idea to attempt to proactively 
 > respond to a
 > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
 > error
 > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
 > Eliminating the lock since the sock is dead anways eliminates the error.
 >
 > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll 
 > try to
 > get the test case to compile.

 It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
 'audit_pid'
 are updated as a whole and race between audit_receive_msg() and
 NETLINK_URELEASE.
>>>
>>> This is what I expected and why I originally added the mutex lock in the
>>> callback...  The dumps I got were bare with no wrapper identifying the
>>> process context or specific error, so I'm at a bit of a loss how to
>>> solve this (without thinking more about it) other than instinctively
>>> removing the mutex.
>>
>> Netlink notifier can safely be converted to blocking one, I will send
>> a patch.
>>
>> But I seriously doubt you really need NETLINK_URELEASE here,
>> it adds nothing but overhead, b/c the netlink notifier is called on
>> every netlink socket in the system, but for net exit path, that is
>> relatively a slow path.
>>
>> Also, kauditd_send_skb() needs audit_cmd_mutex too.
>
> Please let me know what you think about the attached patch?

Applied the patch locally and have not seen the bug since then (~24
hours of testing).

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-09 20:13, Cong Wang wrote:
> On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs  wrote:
> > On 2016-12-08 22:57, Cong Wang wrote:
> >> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  
> >> wrote:
> >> > I also tried to extend Cong Wang's idea to attempt to proactively 
> >> > respond to a
> >> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
> >> > error
> >> > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> >> > Eliminating the lock since the sock is dead anways eliminates the error.
> >> >
> >> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll 
> >> > try to
> >> > get the test case to compile.
> >>
> >> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
> >> 'audit_pid'
> >> are updated as a whole and race between audit_receive_msg() and
> >> NETLINK_URELEASE.
> >
> > This is what I expected and why I originally added the mutex lock in the
> > callback...  The dumps I got were bare with no wrapper identifying the
> > process context or specific error, so I'm at a bit of a loss how to
> > solve this (without thinking more about it) other than instinctively
> > removing the mutex.
> 
> Netlink notifier can safely be converted to blocking one, I will send
> a patch.

I had a quick look at how that might happen.  The netlink notifier chain
is atomic.  Would the registered callback funciton need to spawn a
one-time thread to avoid blocking?

> But I seriously doubt you really need NETLINK_URELEASE here,
> it adds nothing but overhead, b/c the netlink notifier is called on
> every netlink socket in the system, but for net exit path, that is
> relatively a slow path.

I was a bit concerned about its overhead, but was hoping to update
audit_sock more quickly in the case of a sock shutting down for any
reason.

> Also, kauditd_send_skb() needs audit_cmd_mutex too.

Agreed.

> I will send a formal patch.

I had a look at your patch.  It looks attractively simple.  The audit
next tree has patches queued that add an audit_reset function that will
require more work.  I still see some potential gaps.

- If the process messes up (or the sock lookup messes up) it is reset
  in the kauditd thread under the audit_cmd_mutex.

- If the process exits normally or is replaced due to an audit_replace
  error, it is reset from audit_receive_skb under the audit_cmd_mutex.

- If the process dies before the kauditd thread notices, either reap it
  via notifier callback or it needs a check on net exit to reset.  This
  last one appears necessary to decrement the sock refcount so the sock
  can be released in netlink_kernel_release().

If we want to be proactive and use the netlink notifier, we assume the
overhead of adding to the netlink notifier chain and eliminate all the
other reset calls under the kauditd thread.  If we are ok being
reactionary, then we'll at least need the net exit check on audit_sock.

Have I understood this correctly?

I'll follow with a patch based on audit#next

There will be an upstream merge conflict between audit#next and net#next
due to the removal of:
RCU_INIT_POINTER(aunet->nlsk, NULL);

synchronize_net();
from the end of audit_net_exit().  This patch should probably go through
the audit maintainer due to the other anticipated merge conflicts.

> Thanks.

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Fri, Dec 9, 2016 at 8:13 PM, Cong Wang  wrote:
> On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs  wrote:
>> On 2016-12-08 22:57, Cong Wang wrote:
>>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
>>> > I also tried to extend Cong Wang's idea to attempt to proactively respond 
>>> > to a
>>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
>>> > error
>>> > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
>>> > Eliminating the lock since the sock is dead anways eliminates the error.
>>> >
>>> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll 
>>> > try to
>>> > get the test case to compile.
>>>
>>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
>>> 'audit_pid'
>>> are updated as a whole and race between audit_receive_msg() and
>>> NETLINK_URELEASE.
>>
>> This is what I expected and why I originally added the mutex lock in the
>> callback...  The dumps I got were bare with no wrapper identifying the
>> process context or specific error, so I'm at a bit of a loss how to
>> solve this (without thinking more about it) other than instinctively
>> removing the mutex.
>
> Netlink notifier can safely be converted to blocking one, I will send
> a patch.
>
> But I seriously doubt you really need NETLINK_URELEASE here,
> it adds nothing but overhead, b/c the netlink notifier is called on
> every netlink socket in the system, but for net exit path, that is
> relatively a slow path.
>
> Also, kauditd_send_skb() needs audit_cmd_mutex too.

Please let me know what you think about the attached patch?

Thanks!
commit a12b43ee814625933ff155c20dc863c59cfcf240
Author: Cong Wang 
Date:   Fri Dec 9 17:56:42 2016 -0800

audit: close a race condition on audit_sock

Signed-off-by: Cong Wang 

diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..ab947d8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -423,6 +423,8 @@ static void kauditd_send_skb(struct sk_buff *skb)
snprintf(s, sizeof(s), "audit_pid=%d reset", 
audit_pid);
audit_log_lost(s);
audit_pid = 0;
+   audit_nlk_portid = 0;
+   sock_put(audit_sock);
audit_sock = NULL;
} else {
pr_warn("re-scheduling(#%d) write to 
audit_pid=%d\n",
@@ -899,6 +901,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct 
nlmsghdr *nlh)
audit_log_config_change("audit_pid", new_pid, 
audit_pid, 1);
audit_pid = new_pid;
audit_nlk_portid = NETLINK_CB(skb).portid;
+   sock_hold(skb->sk);
+   if (audit_sock)
+   sock_put(audit_sock);
audit_sock = skb->sk;
}
if (s.mask & AUDIT_STATUS_RATE_LIMIT) {
@@ -1167,10 +1172,6 @@ static void __net_exit audit_net_exit(struct net *net)
 {
struct audit_net *aunet = net_generic(net, audit_net_id);
struct sock *sock = aunet->nlsk;
-   if (sock == audit_sock) {
-   audit_pid = 0;
-   audit_sock = NULL;
-   }
 
RCU_INIT_POINTER(aunet->nlsk, NULL);
synchronize_net();

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Fri, Dec 9, 2016 at 3:01 AM, Richard Guy Briggs  wrote:
> On 2016-12-08 22:57, Cong Wang wrote:
>> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
>> > I also tried to extend Cong Wang's idea to attempt to proactively respond 
>> > to a
>> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
>> > error
>> > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
>> > Eliminating the lock since the sock is dead anways eliminates the error.
>> >
>> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll 
>> > try to
>> > get the test case to compile.
>>
>> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 
>> 'audit_pid'
>> are updated as a whole and race between audit_receive_msg() and
>> NETLINK_URELEASE.
>
> This is what I expected and why I originally added the mutex lock in the
> callback...  The dumps I got were bare with no wrapper identifying the
> process context or specific error, so I'm at a bit of a loss how to
> solve this (without thinking more about it) other than instinctively
> removing the mutex.

Netlink notifier can safely be converted to blocking one, I will send
a patch.

But I seriously doubt you really need NETLINK_URELEASE here,
it adds nothing but overhead, b/c the netlink notifier is called on
every netlink socket in the system, but for net exit path, that is
relatively a slow path.

Also, kauditd_send_skb() needs audit_cmd_mutex too.

I will send a formal patch.

Thanks.

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-09 12:53, Dmitry Vyukov wrote:
> On Fri, Dec 9, 2016 at 12:48 PM, Richard Guy Briggs  wrote:
> > On 2016-12-09 11:49, Dmitry Vyukov wrote:
> >> On Fri, Dec 9, 2016 at 7:02 AM, Richard Guy Briggs  wrote:
> >> > On 2016-11-29 23:52, Richard Guy Briggs wrote:
> >> > I tried a quick compile attempt on the test case (I assume it is a
> >> > socket fuzzer) and get the following compile error:
> >> > cc -g -O0 -Wall -D_GNU_SOURCE -o socket_fuzz socket_fuzz.c
> >> > socket_fuzz.c:16:1: warning: "_GNU_SOURCE" redefined
> >> > : warning: this is the location of the previous definition
> >> > socket_fuzz.c: In function ‘segv_handler’:
> >> > socket_fuzz.c:89: warning: implicit declaration of function 
> >> > ‘__atomic_load_n’
> >> > socket_fuzz.c:89: error: ‘__ATOMIC_RELAXED’ undeclared (first use in 
> >> > this function)
> >> > socket_fuzz.c:89: error: (Each undeclared identifier is reported only 
> >> > once
> >> > socket_fuzz.c:89: error: for each function it appears in.)
> >> > socket_fuzz.c: In function ‘loop’:
> >> > socket_fuzz.c:280: warning: unused variable ‘errno0’
> >> > socket_fuzz.c: In function ‘test’:
> >> > socket_fuzz.c:303: warning: implicit declaration of function 
> >> > ‘__atomic_fetch_add’
> >> > socket_fuzz.c:303: error: ‘__ATOMIC_SEQ_CST’ undeclared (first use in 
> >> > this function)
> >> > socket_fuzz.c:303: warning: implicit declaration of function 
> >> > ‘__atomic_fetch_sub’
> >>
> >> -std=gnu99 should help
> >> ignore warnings
> >
> > I got a little further, left with "__ATOMIC_RELAXED undeclared", 
> > "__ATOMIC_SEQ_CST
> > undeclared" under gcc 4.4.7-16.
> >
> > gcc 4.8.2-15 leaves me with "undefined reference to `clock_gettime'"
> 
> add -lrt

Ok, that helped.  Thanks!

> > What compiler version do you recommend?
> 
> 6.x sounds reasonable
> 4.4 branch is 7.5 years old, surprised that it does not disintegrate
> into dust yet :)

  These are under RHEL6...  so there are updates to them, but yeah, they are 
old.

> >> >> - RGB
> >> >
> >> > - RGB
> >
> > - RGB
> >
> > --
> > Richard Guy Briggs 
> > Kernel Security Engineering, Base Operating Systems, Red Hat
> > Remote, Ottawa, Canada
> > Voice: +1.647.777.2635, Internal: (81) 32635

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Dmitry Vyukov]

On Fri, Dec 9, 2016 at 12:48 PM, Richard Guy Briggs  wrote:
> On 2016-12-09 11:49, Dmitry Vyukov wrote:
>> On Fri, Dec 9, 2016 at 7:02 AM, Richard Guy Briggs  wrote:
>> > On 2016-11-29 23:52, Richard Guy Briggs wrote:
>> > I tried a quick compile attempt on the test case (I assume it is a
>> > socket fuzzer) and get the following compile error:
>> > cc -g -O0 -Wall -D_GNU_SOURCE -o socket_fuzz socket_fuzz.c
>> > socket_fuzz.c:16:1: warning: "_GNU_SOURCE" redefined
>> > : warning: this is the location of the previous definition
>> > socket_fuzz.c: In function ‘segv_handler’:
>> > socket_fuzz.c:89: warning: implicit declaration of function 
>> > ‘__atomic_load_n’
>> > socket_fuzz.c:89: error: ‘__ATOMIC_RELAXED’ undeclared (first use in this 
>> > function)
>> > socket_fuzz.c:89: error: (Each undeclared identifier is reported only once
>> > socket_fuzz.c:89: error: for each function it appears in.)
>> > socket_fuzz.c: In function ‘loop’:
>> > socket_fuzz.c:280: warning: unused variable ‘errno0’
>> > socket_fuzz.c: In function ‘test’:
>> > socket_fuzz.c:303: warning: implicit declaration of function 
>> > ‘__atomic_fetch_add’
>> > socket_fuzz.c:303: error: ‘__ATOMIC_SEQ_CST’ undeclared (first use in this 
>> > function)
>> > socket_fuzz.c:303: warning: implicit declaration of function 
>> > ‘__atomic_fetch_sub’
>>
>> -std=gnu99 should help
>> ignore warnings
>
> I got a little further, left with "__ATOMIC_RELAXED undeclared", 
> "__ATOMIC_SEQ_CST
> undeclared" under gcc 4.4.7-16.
>
> gcc 4.8.2-15 leaves me with "undefined reference to `clock_gettime'"

add -lrt


> What compiler version do you recommend?

6.x sounds reasonable
4.4 branch is 7.5 years old, surprised that it does not disintegrate
into dust yet :)


>> >> - RGB
>> >
>> > - RGB
>
> - RGB
>
> --
> Richard Guy Briggs 
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-09 11:49, Dmitry Vyukov wrote:
> On Fri, Dec 9, 2016 at 7:02 AM, Richard Guy Briggs  wrote:
> > On 2016-11-29 23:52, Richard Guy Briggs wrote:
> > I tried a quick compile attempt on the test case (I assume it is a
> > socket fuzzer) and get the following compile error:
> > cc -g -O0 -Wall -D_GNU_SOURCE -o socket_fuzz socket_fuzz.c
> > socket_fuzz.c:16:1: warning: "_GNU_SOURCE" redefined
> > : warning: this is the location of the previous definition
> > socket_fuzz.c: In function ‘segv_handler’:
> > socket_fuzz.c:89: warning: implicit declaration of function 
> > ‘__atomic_load_n’
> > socket_fuzz.c:89: error: ‘__ATOMIC_RELAXED’ undeclared (first use in this 
> > function)
> > socket_fuzz.c:89: error: (Each undeclared identifier is reported only once
> > socket_fuzz.c:89: error: for each function it appears in.)
> > socket_fuzz.c: In function ‘loop’:
> > socket_fuzz.c:280: warning: unused variable ‘errno0’
> > socket_fuzz.c: In function ‘test’:
> > socket_fuzz.c:303: warning: implicit declaration of function 
> > ‘__atomic_fetch_add’
> > socket_fuzz.c:303: error: ‘__ATOMIC_SEQ_CST’ undeclared (first use in this 
> > function)
> > socket_fuzz.c:303: warning: implicit declaration of function 
> > ‘__atomic_fetch_sub’
> 
> -std=gnu99 should help
> ignore warnings

I got a little further, left with "__ATOMIC_RELAXED undeclared", 
"__ATOMIC_SEQ_CST
undeclared" under gcc 4.4.7-16.

gcc 4.8.2-15 leaves me with "undefined reference to `clock_gettime'"

What compiler version do you recommend?

> >> - RGB
> >
> > - RGB

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-12-08 22:57, Cong Wang wrote:
> On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
> > I also tried to extend Cong Wang's idea to attempt to proactively respond 
> > to a
> > NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking 
> > error
> > stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> > Eliminating the lock since the sock is dead anways eliminates the error.
> >
> > Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try 
> > to
> > get the test case to compile.
> 
> It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
> are updated as a whole and race between audit_receive_msg() and
> NETLINK_URELEASE.

This is what I expected and why I originally added the mutex lock in the
callback...  The dumps I got were bare with no wrapper identifying the
process context or specific error, so I'm at a bit of a loss how to
solve this (without thinking more about it) other than instinctively
removing the mutex.

Another approach might be to look at consolidating the three into one
identifier or derive the other two from one, or serialize their access.

> > @@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net 
> > *net)
> >  {
> > struct audit_net *aunet = net_generic(net, audit_net_id);
> > struct sock *sock = aunet->nlsk;
> > +
> > +   mutex_lock(_cmd_mutex);
> > if (sock == audit_sock) {
> > audit_pid = 0;
> > +   audit_nlk_portid = 0;
> > audit_sock = NULL;
> > }
> > +   mutex_unlock(_cmd_mutex);
> 
> If you decide to use NETLINK_URELEASE notifier, the above piece is no
> longer needed, the net_exit path simply releases a refcnt.

Good point.  It would have already killed it off.  So this piece is
arguably too late anyways.

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Dmitry Vyukov]

On Fri, Dec 9, 2016 at 7:02 AM, Richard Guy Briggs  wrote:
> On 2016-11-29 23:52, Richard Guy Briggs wrote:
>> On 2016-11-29 15:13, Cong Wang wrote:
>> > On Tue, Nov 29, 2016 at 8:48 AM, Richard Guy Briggs  
>> > wrote:
>> > > On 2016-11-26 17:11, Cong Wang wrote:
>> > >> It is racy on audit_sock, especially on the netns exit path.
>> > >
>> > > I think that is the only place it is racy.  The other places audit_sock
>> > > is set is when the socket failure has just triggered a reset.
>> > >
>> > > Is there a notifier callback for failed or reaped sockets?
>> >
>> > Is NETLINK_URELEASE event what you are looking for?
>>
>> Possibly, yes.  Thanks, I'll have a look.
>
> I tried a quick compile attempt on the test case (I assume it is a
> socket fuzzer) and get the following compile error:
> cc -g -O0 -Wall -D_GNU_SOURCE -o socket_fuzz socket_fuzz.c
> socket_fuzz.c:16:1: warning: "_GNU_SOURCE" redefined
> : warning: this is the location of the previous definition
> socket_fuzz.c: In function ‘segv_handler’:
> socket_fuzz.c:89: warning: implicit declaration of function ‘__atomic_load_n’
> socket_fuzz.c:89: error: ‘__ATOMIC_RELAXED’ undeclared (first use in this 
> function)
> socket_fuzz.c:89: error: (Each undeclared identifier is reported only once
> socket_fuzz.c:89: error: for each function it appears in.)
> socket_fuzz.c: In function ‘loop’:
> socket_fuzz.c:280: warning: unused variable ‘errno0’
> socket_fuzz.c: In function ‘test’:
> socket_fuzz.c:303: warning: implicit declaration of function 
> ‘__atomic_fetch_add’
> socket_fuzz.c:303: error: ‘__ATOMIC_SEQ_CST’ undeclared (first use in this 
> function)
> socket_fuzz.c:303: warning: implicit declaration of function 
> ‘__atomic_fetch_sub’

-std=gnu99 should help
ignore warnings



> I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> Eliminating the lock since the sock is dead anways eliminates the error.
>
> Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try to
> get the test case to compile.
>
> This is being tracked as https://github.com/linux-audit/audit-kernel/issues/30
>
> Subject: [PATCH] audit: proactively reset audit_sock on matching 
> NETLINK_URELEASE
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..91d222d 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -423,6 +423,7 @@ static void kauditd_send_skb(struct sk_buff *skb)
> snprintf(s, sizeof(s), "audit_pid=%d reset", 
> audit_pid);
> audit_log_lost(s);
> audit_pid = 0;
> +   audit_nlk_portid = 0;
> audit_sock = NULL;
> } else {
> pr_warn("re-scheduling(#%d) write to 
> audit_pid=%d\n",
> @@ -1143,6 +1144,28 @@ static int audit_bind(struct net *net, int group)
> return 0;
>  }
>
> +static int audit_sock_netlink_notify(struct notifier_block *nb,
> +unsigned long event,
> +void *_notify)
> +{
> +   struct netlink_notify *notify = _notify;
> +   struct audit_net *aunet = net_generic(notify->net, audit_net_id);
> +
> +   if (event == NETLINK_URELEASE && notify->protocol == NETLINK_AUDIT) {
> +   if (audit_nlk_portid == notify->portid &&
> +   audit_sock == aunet->nlsk) {
> +   audit_pid = 0;
> +   audit_nlk_portid = 0;
> +   audit_sock = NULL;
> +   }
> +   }
> +   return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block audit_netlink_notifier = {
> +   .notifier_call = audit_sock_netlink_notify,
> +};
> +
>  static int __net_init audit_net_init(struct net *net)
>  {
> struct netlink_kernel_cfg cfg = {
> @@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> +
> +   mutex_lock(_cmd_mutex);
> if (sock == audit_sock) {
> audit_pid = 0;
> +   audit_nlk_portid = 0;
> audit_sock = NULL;
> }
> +   mutex_unlock(_cmd_mutex);
>
> RCU_INIT_POINTER(aunet->nlsk, NULL);
> synchronize_net();
> @@ -1202,6 +1229,7 @@ static int __init audit_init(void)
> audit_enabled = audit_default;
> audit_ever_enabled |= !!audit_default;
>
> +   netlink_register_notifier(_netlink_notifier);
> audit_log(NULL, GFP_KERNEL, AUDIT_KERNEL, "initialized");
>
> for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
> --
> 1.7.1
>
>
>> - RGB
>
> - RGB
>
> --
> Richard Guy Briggs 
> Kernel Security 

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Thu, Dec 8, 2016 at 10:02 PM, Richard Guy Briggs  wrote:
> I also tried to extend Cong Wang's idea to attempt to proactively respond to a
> NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
> stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
> Eliminating the lock since the sock is dead anways eliminates the error.
>
> Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try to
> get the test case to compile.

It doesn't look safe, because 'audit_sock', 'audit_nlk_portid' and 'audit_pid'
are updated as a whole and race between audit_receive_msg() and
NETLINK_URELEASE.


> @@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> +
> +   mutex_lock(_cmd_mutex);
> if (sock == audit_sock) {
> audit_pid = 0;
> +   audit_nlk_portid = 0;
> audit_sock = NULL;
> }
> +   mutex_unlock(_cmd_mutex);
>

If you decide to use NETLINK_URELEASE notifier, the above piece is no
longer needed, the net_exit path simply releases a refcnt.

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-11-29 23:52, Richard Guy Briggs wrote:
> On 2016-11-29 15:13, Cong Wang wrote:
> > On Tue, Nov 29, 2016 at 8:48 AM, Richard Guy Briggs  wrote:
> > > On 2016-11-26 17:11, Cong Wang wrote:
> > >> It is racy on audit_sock, especially on the netns exit path.
> > >
> > > I think that is the only place it is racy.  The other places audit_sock
> > > is set is when the socket failure has just triggered a reset.
> > >
> > > Is there a notifier callback for failed or reaped sockets?
> > 
> > Is NETLINK_URELEASE event what you are looking for?
> 
> Possibly, yes.  Thanks, I'll have a look.

I tried a quick compile attempt on the test case (I assume it is a
socket fuzzer) and get the following compile error:
cc -g -O0 -Wall -D_GNU_SOURCE -o socket_fuzz socket_fuzz.c
socket_fuzz.c:16:1: warning: "_GNU_SOURCE" redefined
: warning: this is the location of the previous definition
socket_fuzz.c: In function ‘segv_handler’:
socket_fuzz.c:89: warning: implicit declaration of function ‘__atomic_load_n’
socket_fuzz.c:89: error: ‘__ATOMIC_RELAXED’ undeclared (first use in this 
function)
socket_fuzz.c:89: error: (Each undeclared identifier is reported only once
socket_fuzz.c:89: error: for each function it appears in.)
socket_fuzz.c: In function ‘loop’:
socket_fuzz.c:280: warning: unused variable ‘errno0’
socket_fuzz.c: In function ‘test’:
socket_fuzz.c:303: warning: implicit declaration of function 
‘__atomic_fetch_add’
socket_fuzz.c:303: error: ‘__ATOMIC_SEQ_CST’ undeclared (first use in this 
function)
socket_fuzz.c:303: warning: implicit declaration of function 
‘__atomic_fetch_sub’

I also tried to extend Cong Wang's idea to attempt to proactively respond to a
NETLINK_URELEASE on the audit_sock and reset it, but ran into a locking error
stack dump using mutex_lock(_cmd_mutex) in the notifier callback.
Eliminating the lock since the sock is dead anways eliminates the error.

Is it safe?  I'll resubmit if this looks remotely sane.  Meanwhile I'll try to
get the test case to compile.

This is being tracked as https://github.com/linux-audit/audit-kernel/issues/30

Subject: [PATCH] audit: proactively reset audit_sock on matching 
NETLINK_URELEASE

diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..91d222d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -423,6 +423,7 @@ static void kauditd_send_skb(struct sk_buff *skb)
snprintf(s, sizeof(s), "audit_pid=%d reset", 
audit_pid);
audit_log_lost(s);
audit_pid = 0;
+   audit_nlk_portid = 0;
audit_sock = NULL;
} else {
pr_warn("re-scheduling(#%d) write to 
audit_pid=%d\n",
@@ -1143,6 +1144,28 @@ static int audit_bind(struct net *net, int group)
return 0;
 }
 
+static int audit_sock_netlink_notify(struct notifier_block *nb,
+unsigned long event,
+void *_notify)
+{
+   struct netlink_notify *notify = _notify;
+   struct audit_net *aunet = net_generic(notify->net, audit_net_id);
+
+   if (event == NETLINK_URELEASE && notify->protocol == NETLINK_AUDIT) {
+   if (audit_nlk_portid == notify->portid &&
+   audit_sock == aunet->nlsk) {
+   audit_pid = 0;
+   audit_nlk_portid = 0;
+   audit_sock = NULL;
+   }
+   }
+   return NOTIFY_DONE;
+}
+
+static struct notifier_block audit_netlink_notifier = {
+   .notifier_call = audit_sock_netlink_notify,
+};
+
 static int __net_init audit_net_init(struct net *net)
 {
struct netlink_kernel_cfg cfg = {
@@ -1167,10 +1190,14 @@ static void __net_exit audit_net_exit(struct net *net)
 {
struct audit_net *aunet = net_generic(net, audit_net_id);
struct sock *sock = aunet->nlsk;
+
+   mutex_lock(_cmd_mutex);
if (sock == audit_sock) {
audit_pid = 0;
+   audit_nlk_portid = 0;
audit_sock = NULL;
}
+   mutex_unlock(_cmd_mutex);
 
RCU_INIT_POINTER(aunet->nlsk, NULL);
synchronize_net();
@@ -1202,6 +1229,7 @@ static int __init audit_init(void)
audit_enabled = audit_default;
audit_ever_enabled |= !!audit_default;
 
+   netlink_register_notifier(_netlink_notifier);
audit_log(NULL, GFP_KERNEL, AUDIT_KERNEL, "initialized");
 
for (i = 0; i < AUDIT_INODE_BUCKETS; i++)
-- 
1.7.1


> - RGB

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-11-29 15:13, Cong Wang wrote:
> On Tue, Nov 29, 2016 at 8:48 AM, Richard Guy Briggs  wrote:
> > On 2016-11-26 17:11, Cong Wang wrote:
> >> It is racy on audit_sock, especially on the netns exit path.
> >
> > I think that is the only place it is racy.  The other places audit_sock
> > is set is when the socket failure has just triggered a reset.
> >
> > Is there a notifier callback for failed or reaped sockets?
> 
> Is NETLINK_URELEASE event what you are looking for?

Possibly, yes.  Thanks, I'll have a look.

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Tue, Nov 29, 2016 at 8:48 AM, Richard Guy Briggs  wrote:
> On 2016-11-26 17:11, Cong Wang wrote:
>> It is racy on audit_sock, especially on the netns exit path.
>
> I think that is the only place it is racy.  The other places audit_sock
> is set is when the socket failure has just triggered a reset.
>
> Is there a notifier callback for failed or reaped sockets?

Is NETLINK_URELEASE event what you are looking for?

Re: netlink: GPF in sock_sndtimeo

[Richard Guy Briggs]

On 2016-11-26 17:11, Cong Wang wrote:
> On Sat, Nov 26, 2016 at 7:44 AM, Dmitry Vyukov  wrote:
> > Hello,

Eric, thanks for the heads up on this.  (more below...)

> > The following program triggers GPF in sock_sndtimeo:
> > https://gist.githubusercontent.com/dvyukov/c19cadd309791cf5cb9b2bf936d3f48d/raw/1743ba0211079a5465d039512b427bc6b59b1a76/gistfile1.txt
> >
> > On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
> >
> > general protection fault:  [#1] SMP DEBUG_PAGEALLOC KASAN
> > Dumping ftrace buffer:
> >(ftrace buffer empty)
> > Modules linked in:
> > CPU: 1 PID: 19950 Comm: syz-executor Not tainted 4.9.0-rc5+ #54
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> > task: 88002a0d0840 task.stack: 88003692
> > RIP: 0010:[]  [< inline >] sock_sndtimeo
> > include/net/sock.h:2075
> > RIP: 0010:[]  []
> > netlink_unicast+0xe1/0x730 net/netlink/af_netlink.c:1232
> > RSP: 0018:880036926f68  EFLAGS: 00010202
> > RAX: 0068 RBX: 880036927000 RCX: c900021d
> > RDX: 0d63 RSI: 024000c0 RDI: 0340
> > RBP: 880036927028 R08: ed0006ea7aab R09: ed0006ea7aab
> > R10: 0001 R11: ed0006ea7aaa R12: dc00
> > R13:  R14: 880035de3400 R15: 880035de3400
> > FS:  7f90a2fc7700() GS:88003ed0() knlGS:
> > CS:  0010 DS:  ES:  CR0: 80050033
> > CR2: 006de0c0 CR3: 35de6000 CR4: 06e0
> > Stack:
> >  880035de3400 819f02a1 110006d24df4 0004
> >  4db40014 880036926fd8  41b58ab3
> >  89653c11 86cb3500 819f0345 880035de3400
> > Call Trace:
> >  [< inline >] audit_replace kernel/audit.c:817
> >  [] audit_receive_msg+0x22c9/0x2ce0 kernel/audit.c:894
> >  [< inline >] audit_receive_skb kernel/audit.c:1120
> >  [] audit_receive+0x1dc/0x360 kernel/audit.c:1133
> >  [< inline >] netlink_unicast_kernel net/netlink/af_netlink.c:1214
> >  [] netlink_unicast+0x514/0x730 
> > net/netlink/af_netlink.c:1240
> >  [] netlink_sendmsg+0xaa4/0xe50 
> > net/netlink/af_netlink.c:1786
> >  [< inline >] sock_sendmsg_nosec net/socket.c:621
> >  [] sock_sendmsg+0xcf/0x110 net/socket.c:631
> >  [] sock_write_iter+0x32b/0x620 net/socket.c:829
> >  [< inline >] new_sync_write fs/read_write.c:499
> >  [] __vfs_write+0x4fe/0x830 fs/read_write.c:512
> >  [] vfs_write+0x175/0x4e0 fs/read_write.c:560
> >  [< inline >] SYSC_write fs/read_write.c:607
> >  [] SyS_write+0x100/0x240 fs/read_write.c:599
> >  [] do_syscall_64+0x2f4/0x940 arch/x86/entry/common.c:280
> >  [] entry_SYSCALL64_slow_path+0x25/0x25
> > Code: fe 4c 89 f7 e8 31 16 ff ff 8b 8d 70 ff ff ff 49 89 c7 31 c0 85
> > c9 75 25 e8 7d 4a a3 fa 49 8d bd 40 03 00 00 48 89 f8 48 c1 e8 03 <42>
> > 80 3c 20 00 0f 85 3a 06 00 00 49 8b 85 40 03 00 00 4c 8d 73
> > RIP  [< inline >] sock_sndtimeo include/net/sock.h:2075
> > RIP  [] netlink_unicast+0xe1/0x730
> > net/netlink/af_netlink.c:1232
> >  RSP 
> > ---[ end trace 8383a15fba6fdc59 ]---
> 
> It is racy on audit_sock, especially on the netns exit path.

I think that is the only place it is racy.  The other places audit_sock
is set is when the socket failure has just triggered a reset.

Is there a notifier callback for failed or reaped sockets?

> Could the following patch help a little bit? Also, I don't see how the
> synchronize_net() here could sync with netlink rcv path, since unlike
> packets from wire, netlink messages are not handled in BH context
> nor I see any RCU taken on rcv path.

Thanks Cong, this looks like it could help.  I'll have a closer look.

> diff --git a/kernel/audit.c b/kernel/audit.c
> index f1ca116..20bc79e 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1167,10 +1167,13 @@ static void __net_exit audit_net_exit(struct net *net)
>  {
> struct audit_net *aunet = net_generic(net, audit_net_id);
> struct sock *sock = aunet->nlsk;
> +
> +   mutex_lock(_cmd_mutex);
> if (sock == audit_sock) {
> audit_pid = 0;
> audit_sock = NULL;
> }
> +   mutex_unlock(_cmd_mutex);
> 
> RCU_INIT_POINTER(aunet->nlsk, NULL);
> synchronize_net();

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

Re: netlink: GPF in sock_sndtimeo

[Cong Wang]

On Sat, Nov 26, 2016 at 7:44 AM, Dmitry Vyukov  wrote:
> Hello,
>
> The following program triggers GPF in sock_sndtimeo:
> https://gist.githubusercontent.com/dvyukov/c19cadd309791cf5cb9b2bf936d3f48d/raw/1743ba0211079a5465d039512b427bc6b59b1a76/gistfile1.txt
>
> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>
> general protection fault:  [#1] SMP DEBUG_PAGEALLOC KASAN
> Dumping ftrace buffer:
>(ftrace buffer empty)
> Modules linked in:
> CPU: 1 PID: 19950 Comm: syz-executor Not tainted 4.9.0-rc5+ #54
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: 88002a0d0840 task.stack: 88003692
> RIP: 0010:[]  [< inline >] sock_sndtimeo
> include/net/sock.h:2075
> RIP: 0010:[]  []
> netlink_unicast+0xe1/0x730 net/netlink/af_netlink.c:1232
> RSP: 0018:880036926f68  EFLAGS: 00010202
> RAX: 0068 RBX: 880036927000 RCX: c900021d
> RDX: 0d63 RSI: 024000c0 RDI: 0340
> RBP: 880036927028 R08: ed0006ea7aab R09: ed0006ea7aab
> R10: 0001 R11: ed0006ea7aaa R12: dc00
> R13:  R14: 880035de3400 R15: 880035de3400
> FS:  7f90a2fc7700() GS:88003ed0() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 006de0c0 CR3: 35de6000 CR4: 06e0
> Stack:
>  880035de3400 819f02a1 110006d24df4 0004
>  4db40014 880036926fd8  41b58ab3
>  89653c11 86cb3500 819f0345 880035de3400
> Call Trace:
>  [< inline >] audit_replace kernel/audit.c:817
>  [] audit_receive_msg+0x22c9/0x2ce0 kernel/audit.c:894
>  [< inline >] audit_receive_skb kernel/audit.c:1120
>  [] audit_receive+0x1dc/0x360 kernel/audit.c:1133
>  [< inline >] netlink_unicast_kernel net/netlink/af_netlink.c:1214
>  [] netlink_unicast+0x514/0x730 
> net/netlink/af_netlink.c:1240
>  [] netlink_sendmsg+0xaa4/0xe50 
> net/netlink/af_netlink.c:1786
>  [< inline >] sock_sendmsg_nosec net/socket.c:621
>  [] sock_sendmsg+0xcf/0x110 net/socket.c:631
>  [] sock_write_iter+0x32b/0x620 net/socket.c:829
>  [< inline >] new_sync_write fs/read_write.c:499
>  [] __vfs_write+0x4fe/0x830 fs/read_write.c:512
>  [] vfs_write+0x175/0x4e0 fs/read_write.c:560
>  [< inline >] SYSC_write fs/read_write.c:607
>  [] SyS_write+0x100/0x240 fs/read_write.c:599
>  [] do_syscall_64+0x2f4/0x940 arch/x86/entry/common.c:280
>  [] entry_SYSCALL64_slow_path+0x25/0x25
> Code: fe 4c 89 f7 e8 31 16 ff ff 8b 8d 70 ff ff ff 49 89 c7 31 c0 85
> c9 75 25 e8 7d 4a a3 fa 49 8d bd 40 03 00 00 48 89 f8 48 c1 e8 03 <42>
> 80 3c 20 00 0f 85 3a 06 00 00 49 8b 85 40 03 00 00 4c 8d 73
> RIP  [< inline >] sock_sndtimeo include/net/sock.h:2075
> RIP  [] netlink_unicast+0xe1/0x730
> net/netlink/af_netlink.c:1232
>  RSP 
> ---[ end trace 8383a15fba6fdc59 ]---

It is racy on audit_sock, especially on the netns exit path.

Could the following patch help a little bit? Also, I don't see how the
synchronize_net() here could sync with netlink rcv path, since unlike
packets from wire, netlink messages are not handled in BH context
nor I see any RCU taken on rcv path.

diff --git a/kernel/audit.c b/kernel/audit.c
index f1ca116..20bc79e 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1167,10 +1167,13 @@ static void __net_exit audit_net_exit(struct net *net)
 {
struct audit_net *aunet = net_generic(net, audit_net_id);
struct sock *sock = aunet->nlsk;
+
+   mutex_lock(_cmd_mutex);
if (sock == audit_sock) {
audit_pid = 0;
audit_sock = NULL;
}
+   mutex_unlock(_cmd_mutex);

RCU_INIT_POINTER(aunet->nlsk, NULL);
synchronize_net();

Re: netlink: GPF in sock_sndtimeo

[Eric Dumazet]

CC Richard Guy Briggs 

On Sat, Nov 26, 2016 at 7:44 AM, Dmitry Vyukov  wrote:
> Hello,
>
> The following program triggers GPF in sock_sndtimeo:
> https://gist.githubusercontent.com/dvyukov/c19cadd309791cf5cb9b2bf936d3f48d/raw/1743ba0211079a5465d039512b427bc6b59b1a76/gistfile1.txt
>
> On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).
>
> general protection fault:  [#1] SMP DEBUG_PAGEALLOC KASAN
> Dumping ftrace buffer:
>(ftrace buffer empty)
> Modules linked in:
> CPU: 1 PID: 19950 Comm: syz-executor Not tainted 4.9.0-rc5+ #54
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> task: 88002a0d0840 task.stack: 88003692
> RIP: 0010:[]  [< inline >] sock_sndtimeo
> include/net/sock.h:2075
> RIP: 0010:[]  []
> netlink_unicast+0xe1/0x730 net/netlink/af_netlink.c:1232
> RSP: 0018:880036926f68  EFLAGS: 00010202
> RAX: 0068 RBX: 880036927000 RCX: c900021d
> RDX: 0d63 RSI: 024000c0 RDI: 0340
> RBP: 880036927028 R08: ed0006ea7aab R09: ed0006ea7aab
> R10: 0001 R11: ed0006ea7aaa R12: dc00
> R13:  R14: 880035de3400 R15: 880035de3400
> FS:  7f90a2fc7700() GS:88003ed0() knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 006de0c0 CR3: 35de6000 CR4: 06e0
> Stack:
>  880035de3400 819f02a1 110006d24df4 0004
>  4db40014 880036926fd8  41b58ab3
>  89653c11 86cb3500 819f0345 880035de3400
> Call Trace:
>  [< inline >] audit_replace kernel/audit.c:817
>  [] audit_receive_msg+0x22c9/0x2ce0 kernel/audit.c:894
>  [< inline >] audit_receive_skb kernel/audit.c:1120
>  [] audit_receive+0x1dc/0x360 kernel/audit.c:1133
>  [< inline >] netlink_unicast_kernel net/netlink/af_netlink.c:1214
>  [] netlink_unicast+0x514/0x730 
> net/netlink/af_netlink.c:1240
>  [] netlink_sendmsg+0xaa4/0xe50 
> net/netlink/af_netlink.c:1786
>  [< inline >] sock_sendmsg_nosec net/socket.c:621
>  [] sock_sendmsg+0xcf/0x110 net/socket.c:631
>  [] sock_write_iter+0x32b/0x620 net/socket.c:829
>  [< inline >] new_sync_write fs/read_write.c:499
>  [] __vfs_write+0x4fe/0x830 fs/read_write.c:512
>  [] vfs_write+0x175/0x4e0 fs/read_write.c:560
>  [< inline >] SYSC_write fs/read_write.c:607
>  [] SyS_write+0x100/0x240 fs/read_write.c:599
>  [] do_syscall_64+0x2f4/0x940 arch/x86/entry/common.c:280
>  [] entry_SYSCALL64_slow_path+0x25/0x25
> Code: fe 4c 89 f7 e8 31 16 ff ff 8b 8d 70 ff ff ff 49 89 c7 31 c0 85
> c9 75 25 e8 7d 4a a3 fa 49 8d bd 40 03 00 00 48 89 f8 48 c1 e8 03 <42>
> 80 3c 20 00 0f 85 3a 06 00 00 49 8b 85 40 03 00 00 4c 8d 73
> RIP  [< inline >] sock_sndtimeo include/net/sock.h:2075
> RIP  [] netlink_unicast+0xe1/0x730
> net/netlink/af_netlink.c:1232
>  RSP 
> ---[ end trace 8383a15fba6fdc59 ]---


Looks a bug added in commit 32a1dbaece7e37cea415e03cd426172249aa859e
("audit: try harder to send to auditd upon netlink failure")
or 133e1e5acd4a63c4a0dcc413e90d5decdbce9c4a ("audit: stop an old
auditd being starved out by a new auditd")

Richard, can you take a look ?

Thanks !

netlink: GPF in sock_sndtimeo

[Dmitry Vyukov]

Hello,

The following program triggers GPF in sock_sndtimeo:
https://gist.githubusercontent.com/dvyukov/c19cadd309791cf5cb9b2bf936d3f48d/raw/1743ba0211079a5465d039512b427bc6b59b1a76/gistfile1.txt

On commit 16ae16c6e5616c084168740990fc508bda6655d4 (Nov 24).

general protection fault:  [#1] SMP DEBUG_PAGEALLOC KASAN
Dumping ftrace buffer:
   (ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 19950 Comm: syz-executor Not tainted 4.9.0-rc5+ #54
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: 88002a0d0840 task.stack: 88003692
RIP: 0010:[]  [< inline >] sock_sndtimeo
include/net/sock.h:2075
RIP: 0010:[]  []
netlink_unicast+0xe1/0x730 net/netlink/af_netlink.c:1232
RSP: 0018:880036926f68  EFLAGS: 00010202
RAX: 0068 RBX: 880036927000 RCX: c900021d
RDX: 0d63 RSI: 024000c0 RDI: 0340
RBP: 880036927028 R08: ed0006ea7aab R09: ed0006ea7aab
R10: 0001 R11: ed0006ea7aaa R12: dc00
R13:  R14: 880035de3400 R15: 880035de3400
FS:  7f90a2fc7700() GS:88003ed0() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 006de0c0 CR3: 35de6000 CR4: 06e0
Stack:
 880035de3400 819f02a1 110006d24df4 0004
 4db40014 880036926fd8  41b58ab3
 89653c11 86cb3500 819f0345 880035de3400
Call Trace:
 [< inline >] audit_replace kernel/audit.c:817
 [] audit_receive_msg+0x22c9/0x2ce0 kernel/audit.c:894
 [< inline >] audit_receive_skb kernel/audit.c:1120
 [] audit_receive+0x1dc/0x360 kernel/audit.c:1133
 [< inline >] netlink_unicast_kernel net/netlink/af_netlink.c:1214
 [] netlink_unicast+0x514/0x730 net/netlink/af_netlink.c:1240
 [] netlink_sendmsg+0xaa4/0xe50 net/netlink/af_netlink.c:1786
 [< inline >] sock_sendmsg_nosec net/socket.c:621
 [] sock_sendmsg+0xcf/0x110 net/socket.c:631
 [] sock_write_iter+0x32b/0x620 net/socket.c:829
 [< inline >] new_sync_write fs/read_write.c:499
 [] __vfs_write+0x4fe/0x830 fs/read_write.c:512
 [] vfs_write+0x175/0x4e0 fs/read_write.c:560
 [< inline >] SYSC_write fs/read_write.c:607
 [] SyS_write+0x100/0x240 fs/read_write.c:599
 [] do_syscall_64+0x2f4/0x940 arch/x86/entry/common.c:280
 [] entry_SYSCALL64_slow_path+0x25/0x25
Code: fe 4c 89 f7 e8 31 16 ff ff 8b 8d 70 ff ff ff 49 89 c7 31 c0 85
c9 75 25 e8 7d 4a a3 fa 49 8d bd 40 03 00 00 48 89 f8 48 c1 e8 03 <42>
80 3c 20 00 0f 85 3a 06 00 00 49 8b 85 40 03 00 00 4c 8d 73
RIP  [< inline >] sock_sndtimeo include/net/sock.h:2075
RIP  [] netlink_unicast+0xe1/0x730
net/netlink/af_netlink.c:1232
 RSP 
---[ end trace 8383a15fba6fdc59 ]---