[PATCH] netlink: do not set cb_running if dump's start() errs

[PATCH] netlink: do not set cb_running if dump's start() errs

[Jason A. Donenfeld]

It turns out that multiple places can call netlink_dump(), which means
it's still possible to dereference partially initialized values in
dump() that were the result of a faulty returned start().

This fixes the issue by calling start() _before_ setting cb_running to
true, so that there's no chance at all of hitting the dump() function
through any indirect paths.

In testing this with several different pieces of tricky code to trigger
these issues, this commit fixes all avenues that I'm aware of.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
 net/netlink/af_netlink.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 94c11cf0459d..f34750691c5c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2266,16 +2266,17 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	cb->min_dump_alloc = control->min_dump_alloc;
 	cb->skb = skb;
 
+	if (cb->start) {
+		ret = cb->start(cb);
+		if (ret)
+			goto error_unlock;
+	}
+
 	nlk->cb_running = true;
 
 	mutex_unlock(nlk->cb_mutex);
 
-	ret = 0;
-	if (cb->start)
-		ret = cb->start(cb);
-
-	if (!ret)
-		ret = netlink_dump(sk);
+	ret = netlink_dump(sk);
 
 	sock_put(sk);
 
-- 
2.14.2

Re: [PATCH] netlink: do not set cb_running if dump's start() errs

[Jason A. Donenfeld]

Dave - this very likely should be queued up for stable.

Jason

Re: [PATCH] netlink: do not set cb_running if dump's start() errs

[Johannes Berg]

On Mon, 2017-10-09 at 13:56 +0200, Jason A. Donenfeld wrote:

> @@ -2266,16 +2266,17 @@ int __netlink_dump_start(struct sock *ssk,
> struct sk_buff *skb,
>  	cb->min_dump_alloc = control->min_dump_alloc;
>  	cb->skb = skb;
>  
> +	if (cb->start) {
> +		ret = cb->start(cb);
> +		if (ret)
> +			goto error_unlock;
> +	}
> +
>  	nlk->cb_running = true;
>  
>  	mutex_unlock(nlk->cb_mutex);

Hmm. Now start is invoked with the mutex held, I'm not sure it actually
_matters_, but that should probably be reviewed and mentioned in the
commit log?

johannes

Re: [PATCH] netlink: do not set cb_running if dump's start() errs

[Jason A. Donenfeld]

Hi Johannes,

Yes, indeed, and I think that's actually a good thing. It means that
the starts aren't ever called in parallel, which could be useful if
drivers have any ordering requirements. The change doesn't negatively
effect any existing drivers. I'll resubmit with a larger commit
message explaining this.

Jason

[PATCH v2] netlink: do not set cb_running if dump's start() errs

[Jason A. Donenfeld]

It turns out that multiple places can call netlink_dump(), which means
it's still possible to dereference partially initialized values in
dump() that were the result of a faulty returned start().

This fixes the issue by calling start() _before_ setting cb_running to
true, so that there's no chance at all of hitting the dump() function
through any indirect paths.

It also moves the call to start() to be when the mutex is held. This has
the nice side effect of serializing invocations to start(), which is
likely desirable anyway. It also prevents any possible other races that
might come out of this logic.

In testing this with several different pieces of tricky code to trigger
these issues, this commit fixes all avenues that I'm aware of.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
---
 net/netlink/af_netlink.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 94c11cf0459d..f34750691c5c 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2266,16 +2266,17 @@ int __netlink_dump_start(struct sock *ssk, struct sk_buff *skb,
 	cb->min_dump_alloc = control->min_dump_alloc;
 	cb->skb = skb;
 
+	if (cb->start) {
+		ret = cb->start(cb);
+		if (ret)
+			goto error_unlock;
+	}
+
 	nlk->cb_running = true;
 
 	mutex_unlock(nlk->cb_mutex);
 
-	ret = 0;
-	if (cb->start)
-		ret = cb->start(cb);
-
-	if (!ret)
-		ret = netlink_dump(sk);
+	ret = netlink_dump(sk);
 
 	sock_put(sk);
 
-- 
2.14.2

Re: [PATCH v2] netlink: do not set cb_running if dump's start() errs

[Johannes Berg]

On Mon, 2017-10-09 at 14:14 +0200, Jason A. Donenfeld wrote:
> It turns out that multiple places can call netlink_dump(), which
> means
> it's still possible to dereference partially initialized values in
> dump() that were the result of a faulty returned start().
> 
> This fixes the issue by calling start() _before_ setting cb_running
> to
> true, so that there's no chance at all of hitting the dump() function
> through any indirect paths.
> 
> It also moves the call to start() to be when the mutex is held. This
> has
> the nice side effect of serializing invocations to start(), which is
> likely desirable anyway. It also prevents any possible other races
> that
> might come out of this logic.

I'm not necessarily sure it's _nice_, but I do think it doesn't matter,
so that's just splitting hairs. If you do have a genl family with
parallel_ops, you'd better be prepared to handle parallel things, and
then this could also be in parallel :-)

> In testing this with several different pieces of tricky code to
> trigger
> these issues, this commit fixes all avenues that I'm aware of.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

johannes

Re: [PATCH v2] netlink: do not set cb_running if dump's start() errs

[Jason A. Donenfeld]

On Mon, Oct 9, 2017 at 2:31 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Reviewed-by: Johannes Berg <johannes@sipsolutions.net>

Thanks for the review. Hopefully this can make it into 4.13.6 and 4.14-rc5.

Re: [PATCH v2] netlink: do not set cb_running if dump's start() errs

[David Miller]

From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Mon,  9 Oct 2017 14:14:51 +0200

> It turns out that multiple places can call netlink_dump(), which means
> it's still possible to dereference partially initialized values in
> dump() that were the result of a faulty returned start().
> 
> This fixes the issue by calling start() _before_ setting cb_running to
> true, so that there's no chance at all of hitting the dump() function
> through any indirect paths.
> 
> It also moves the call to start() to be when the mutex is held. This has
> the nice side effect of serializing invocations to start(), which is
> likely desirable anyway. It also prevents any possible other races that
> might come out of this logic.
> 
> In testing this with several different pieces of tricky code to trigger
> these issues, this commit fixes all avenues that I'm aware of.
> 
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

Applied and queued up for -stable.

Re: [PATCH] netlink: do not set cb_running if dump's start() errs

[Johannes Berg]

Just decided to take another look:

On Mon, 2017-10-09 at 13:58 +0200, Johannes Berg wrote:
> On Mon, 2017-10-09 at 13:56 +0200, Jason A. Donenfeld wrote:
> 
> > @@ -2266,16 +2266,17 @@ int __netlink_dump_start(struct sock *ssk,
> > struct sk_buff *skb,
> >  	cb->min_dump_alloc = control->min_dump_alloc;
> >  	cb->skb = skb;
> >  
> > +	if (cb->start) {
> > +		ret = cb->start(cb);
> > +		if (ret)
> > +			goto error_unlock;
> > +	}
> > +
> >  	nlk->cb_running = true;
> >  
> >  	mutex_unlock(nlk->cb_mutex);
> 
> Hmm. Now start is invoked with the mutex held, I'm not sure it
> actually _matters_, but that should probably be reviewed and
> mentioned in the commit log?

It sort of seems designed to run ->start outside the lock, otherwise we
wouldn't really have to acquire it again in netlink_dump() but could
just keep it across the call (with some locking changes in
netlink_recvmsg())?

Then again, clearly none of the (few) existing users actually care.

Btw - we should (separately) also remove "start" from struct
netlink_callback, it's only ever used within this function and we can
use control->start instead of cb->start here.

johannes