[PATCH] rds: fix use-after-free read in rds_find_bound

socket buffer can get freed as part of sock_close
callback so before adding reference check underneath
socket validity.

Reported-by: syzbot+93a5839deb355537440f@syzkaller.appspotmail.com
Signed-off-by: Santosh Shilimkar <redacted>
---
 net/rds/bind.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rds/bind.c b/net/rds/bind.c
index 75d43dc..8dec06e 100644
--- a/net/rds/bind.c
+++ b/net/rds/bind.c

@@ -61,7 +61,7 @@ struct rds_sock *rds_find_bound(__be32 addr, __be16 port)
 	struct rds_sock *rs;
 
 	rs = rhashtable_lookup_fast(&bind_hash_table, &key, ht_parms);
-	if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
+	if (rs && rds_rs_to_sk(rs) && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
 		rds_sock_addref(rs);
 	else
 		rs = NULL;

-- 
1.9.1

On (12/30/17 11:36), Santosh Shilimkar wrote:

socket buffer can get freed as part of sock_close
callback so before adding reference check underneath
socket validity.

I'm not sure I understand this fix-  

struct rds_sock is:
  struct rds_sock {
        struct sock             rs_sk;
         :
  }

How can  rs be non-null but rds_rs_to_sk() is null? (Note that
rds_rs_to_sk just returns &rs->rs_sk) so the changed line is 
identical to the original line.

-	if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
+	if (rs && rds_rs_to_sk(rs) && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))

I think the real issue is refcount bug somewhere,

Was the syzbot test run with http://patchwork.ozlabs.org/patch/852492/
this sounds like that type of bug.

--Sowmini

On 12/30/17 12:26 PM, Sowmini Varadhan wrote:

On (12/30/17 11:36), Santosh Shilimkar wrote:

quoted

socket buffer can get freed as part of sock_close
callback so before adding reference check underneath
socket validity.

I'm not sure I understand this fix-

struct rds_sock is:
   struct rds_sock {
         struct sock             rs_sk;
          :
   }

How can  rs be non-null but rds_rs_to_sk() is null? (Note that
rds_rs_to_sk just returns &rs->rs_sk) so the changed line is
identical to the original line.

Well thats what the report says o.w flag test wouldn't have
been attempted.

quoted

-	if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
+	if (rs && rds_rs_to_sk(rs) && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))

I think the real issue is refcount bug somewhere,

Thats what I thought as well initially but since the reported case,
the rs seems to be valid where as sk seems to be freed up as part of
sock_release callback.

Was the syzbot test run with http://patchwork.ozlabs.org/patch/852492/
this sounds like that type of bug.

That fix scenario, the rs don't get inserted in hash table and
in this particular bug, the lookup was successful so am not sure
if these two bugs are related.

But since bound address fix was still not part of the build
reproduced use after free bug, $subject fix can wait for next
reproduction. Unfortunately as per the report, there is no
reproducer for it to test if other fix fixes this issue.

Regards,
Santosh

On (12/30/17 13:37), santosh.shilimkar@oracle.com wrote:

Well thats what the report says o.w flag test wouldn't have
been attempted.

the bug report says "use-after-free". 

It doesnt say that rds_rs_to_sk(rs) is null (if rds_rs_to_sk(rs) was null,
rs would also be null, please cscope struct rds_sock)

What the bug report says is 
" The buggy address belongs to the object at ffff8801c09a6080
  which belongs to the cache RDS of size 1472
 The buggy address is located 96 bytes inside of .."

96 is the offset of sk->sk_flags. so yes, there is a socket refcount
issue.

But the patch you sent (see next two lines) will not solve that.

quoted

quoted

-	if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
+	if (rs && rds_rs_to_sk(rs) && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))

Sowmini>I think the real issue is refcount bug somewhere,

Thats what I thought as well initially but since the reported case,
the rs seems to be valid where as sk seems to be freed up as part of
sock_release callback.

I dont understand the statement above- how can "rs be valid, and sk
be freed"?

rs_sk is embedded in the struct rds_sock, it is not a pointer. 

let's find and fix the refcount bug. See stack trace in commit comment.
The socket release is happening prematurely and existing WARN_ONs
are not catching it.

quoted

Was the syzbot test run with http://patchwork.ozlabs.org/patch/852492/
this sounds like that type of bug.

--Sowmini

On 12/30/17 2:32 PM, Sowmini Varadhan wrote:

On (12/30/17 13:37), santosh.shilimkar@oracle.com wrote:

[...]

quoted

Thats what I thought as well initially but since the reported case,
the rs seems to be valid where as sk seems to be freed up as part of
sock_release callback.

I dont understand the statement above- how can "rs be valid, and sk
be freed"?

rs_sk is embedded in the struct rds_sock, it is not a pointer.

I was going with order of evaluation of if () but you made good point.
rs_sk isn't a pointer so sk can't be null.

let's find and fix the refcount bug. See stack trace in commit comment.
The socket release is happening prematurely and existing WARN_ONs
are not catching it.

Right. This was loop transport in action so xmit will just flip
the direction with receive. And rds_recv_incoming() can race with socket_release. rds_find_bound() is suppose to add ref count on
socket for rds_recv_incoming() but by that time socket is DEAD &
freed by socket release callback.
And rds_release is suppose to be synced with rs_recv_lock. But
release callback is marking the sk orphan before syncing
up with receive path and updating the bind table. Probably it
can pushed down further after the socket clean up buut need
to think bit more.

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index b405f77..11e1426 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c

@@ -65,7 +65,6 @@ static int rds_release(struct socket *sock)

        rs = rds_sk_to_rs(sk);

-       sock_orphan(sk);
        /* Note - rds_clear_recv_queue grabs rs_recv_lock, so
         * that ensures the recv path has completed messing
         * with the socket. */

@@ -85,6 +84,7 @@ static int rds_release(struct socket *sock)

        rds_trans_put(rs->rs_transport);

+       sock_orphan(sk);
        sock->sk = NULL;
        sock_put(sk);
 out:

On (12/30/17 21:09), santosh.shilimkar@oracle.com wrote:

Right. This was loop transport in action so xmit will just flip
the direction with receive. And rds_recv_incoming() can race with
socket_release. rds_find_bound() is suppose to add ref count on
socket for rds_recv_incoming() but by that time socket is DEAD &
freed by socket release callback.

correct, that makes sense.

And rds_release is suppose to be synced with rs_recv_lock. But
release callback is marking the sk orphan before syncing
up with receive path and updating the bind table. Probably it
can pushed down further after the socket clean up buut need
to think bit more.

However, I'm not sure this seals the race..  according to the
bug report rds_recv_incoming->rds_find_bound is being called
in rds_send_worker context and the rds_find_bound code is

     63         rs = rhashtable_lookup_fast(&bind_hash_table, &key, ht_parms);
     64         if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
     65                 rds_sock_addref(rs);
     66         else 
     67                 rs = NULL;
     68 

Since the entire logic of rds_release can interleave between line 63
and 64, (whereas we only addref at line 65), moving the sock_orphan
will not help.

I see that there was an explicic synchornization via the bucket->lock 
before 7b5654349e. I think you need something like that, or some type
or rcu-based hash list.

Patch below may make race-window smaller, but race window is still there.

On 12/31/17 4:33 AM, Sowmini Varadhan wrote:

On (12/30/17 21:09), santosh.shilimkar@oracle.com wrote:

quoted

Right. This was loop transport in action so xmit will just flip
the direction with receive. And rds_recv_incoming() can race with
socket_release. rds_find_bound() is suppose to add ref count on
socket for rds_recv_incoming() but by that time socket is DEAD &
freed by socket release callback.

correct, that makes sense.

Yea. In fact the earlier point of sk being null and rs not is
also possible because socket release explicitly marks its
NULL ("sock->sk = NULL"). But it just side effect of the actual
race.

quoted

And rds_release is suppose to be synced with rs_recv_lock. But
release callback is marking the sk orphan before syncing
up with receive path and updating the bind table. Probably it
can pushed down further after the socket clean up buut need
to think bit more.

However, I'm not sure this seals the race..  according to the
bug report rds_recv_incoming->rds_find_bound is being called
in rds_send_worker context and the rds_find_bound code is

      63         rs = rhashtable_lookup_fast(&bind_hash_table, &key, ht_parms);
      64         if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
      65                 rds_sock_addref(rs);
      66         else
      67                 rs = NULL;
      68

Since the entire logic of rds_release can interleave between line 63
and 64, (whereas we only addref at line 65), moving the sock_orphan
will not help.

I see that there was an explicic synchornization via the bucket->lock
before 7b5654349e. I think you need something like that, or some type
or rcu-based hash list.

The rhashtable already has internal bucket lock so those operation like
add/remove are synced up.  But yes reference addition can still race
with receive since receive lock is taken after find bound.

Patch below may make race-window smaller, but race window is still there.
If the receive lock is taken ahead then with sock_orphan

change socket release will get synchronized with receive. But
preventing socket release to be getting triggered while in receive
path by means ref count is better option. Moving socket_orphan
down is anyway a good change but its not enough. Will think bit
more about it.

Thanks for the good discussion.

Regards,
Santosh