Re: [PATCH net] sctp: call rcu_read_lock before checking for duplicate transport nodes

[David Miller]

From: Xin Long 
Date: Tue, 28 Feb 2017 12:41:29 +0800

> Commit cd2b70875058 ("sctp: check duplicate node before inserting a
> new transport") called rhltable_lookup() to check for the duplicate
> transport node in transport rhashtable.
> 
> But rhltable_lookup() doesn't call rcu_read_lock inside, it could cause
> a use-after-free issue if it tries to dereference the node that another
> cpu has freed it. Note that sock lock can not avoid this as it is per
> sock.
> 
> This patch is to fix it by calling rcu_read_lock before checking for
> duplicate transport nodes.
> 
> Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new 
> transport")
> Reported-by: Andrey Konovalov 
> Signed-off-by: Xin Long 

Applied.

Re: [PATCH net] sctp: call rcu_read_lock before checking for duplicate transport nodes

[Neil Horman]

On Tue, Feb 28, 2017 at 10:37:35PM +0800, Xin Long wrote:
> On Tue, Feb 28, 2017 at 10:23 PM, Neil Horman  wrote:
> > On Tue, Feb 28, 2017 at 12:41:29PM +0800, Xin Long wrote:
> >> Commit cd2b70875058 ("sctp: check duplicate node before inserting a
> >> new transport") called rhltable_lookup() to check for the duplicate
> >> transport node in transport rhashtable.
> >>
> >> But rhltable_lookup() doesn't call rcu_read_lock inside, it could cause
> >> a use-after-free issue if it tries to dereference the node that another
> >> cpu has freed it. Note that sock lock can not avoid this as it is per
> >> sock.
> >>
> >> This patch is to fix it by calling rcu_read_lock before checking for
> >> duplicate transport nodes.
> >>
> >> Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new 
> >> transport")
> >> Reported-by: Andrey Konovalov 
> >> Signed-off-by: Xin Long 
> >> ---
> >>  net/sctp/input.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/net/sctp/input.c b/net/sctp/input.c
> >> index fc45896..2a28ab2 100644
> >> --- a/net/sctp/input.c
> >> +++ b/net/sctp/input.c
> >> @@ -884,14 +884,17 @@ int sctp_hash_transport(struct sctp_transport *t)
> >>   arg.paddr = >ipaddr;
> >>   arg.lport = htons(t->asoc->base.bind_addr.port);
> >>
> >> + rcu_read_lock();
> >>   list = rhltable_lookup(_transport_hashtable, ,
> >>  sctp_hash_params);
> >>
> >>   rhl_for_each_entry_rcu(transport, tmp, list, node)
> >>   if (transport->asoc->ep == t->asoc->ep) {
> >> + rcu_read_unlock();
> >>   err = -EEXIST;
> >>   goto out;
> >>   }
> >> + rcu_read_unlock();
> >>
> >>   err = rhltable_insert_key(_transport_hashtable, ,
> >> >node, sctp_hash_params);
> >> --
> >> 2.1.0
> >>
> >>
> >
> > Hmm, rhltable_insert_key I think returns a pointer to the existing object 
> > in the
> > hash table if there is a collision (according to the _rhashtable_insert_fast
> > description).  Wouldn't it be better to eliminate the rhl_for_each_entry_rcu
> > entirely, and just check the returned pointer from rhltable_insert_key?
> I wish it could do it, but rhlist(rhltable_insert_key) allows duplicate
> node to be inserted, it will never return the existing node. :(
> 
> I guess the codes you saw are only for the old rhashtable api (they are
> together with rhlist codes in __rhashtable_insert_fast).
> 
> Thanks
> 
yeah, you're right, can't do it that way

Acked-by: Neil Horman 

> >
> > Neil
> >
> 

Re: [PATCH net] sctp: call rcu_read_lock before checking for duplicate transport nodes

[Xin Long]

On Tue, Feb 28, 2017 at 10:23 PM, Neil Horman  wrote:
> On Tue, Feb 28, 2017 at 12:41:29PM +0800, Xin Long wrote:
>> Commit cd2b70875058 ("sctp: check duplicate node before inserting a
>> new transport") called rhltable_lookup() to check for the duplicate
>> transport node in transport rhashtable.
>>
>> But rhltable_lookup() doesn't call rcu_read_lock inside, it could cause
>> a use-after-free issue if it tries to dereference the node that another
>> cpu has freed it. Note that sock lock can not avoid this as it is per
>> sock.
>>
>> This patch is to fix it by calling rcu_read_lock before checking for
>> duplicate transport nodes.
>>
>> Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new 
>> transport")
>> Reported-by: Andrey Konovalov 
>> Signed-off-by: Xin Long 
>> ---
>>  net/sctp/input.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/net/sctp/input.c b/net/sctp/input.c
>> index fc45896..2a28ab2 100644
>> --- a/net/sctp/input.c
>> +++ b/net/sctp/input.c
>> @@ -884,14 +884,17 @@ int sctp_hash_transport(struct sctp_transport *t)
>>   arg.paddr = >ipaddr;
>>   arg.lport = htons(t->asoc->base.bind_addr.port);
>>
>> + rcu_read_lock();
>>   list = rhltable_lookup(_transport_hashtable, ,
>>  sctp_hash_params);
>>
>>   rhl_for_each_entry_rcu(transport, tmp, list, node)
>>   if (transport->asoc->ep == t->asoc->ep) {
>> + rcu_read_unlock();
>>   err = -EEXIST;
>>   goto out;
>>   }
>> + rcu_read_unlock();
>>
>>   err = rhltable_insert_key(_transport_hashtable, ,
>> >node, sctp_hash_params);
>> --
>> 2.1.0
>>
>>
>
> Hmm, rhltable_insert_key I think returns a pointer to the existing object in 
> the
> hash table if there is a collision (according to the _rhashtable_insert_fast
> description).  Wouldn't it be better to eliminate the rhl_for_each_entry_rcu
> entirely, and just check the returned pointer from rhltable_insert_key?
I wish it could do it, but rhlist(rhltable_insert_key) allows duplicate
node to be inserted, it will never return the existing node. :(

I guess the codes you saw are only for the old rhashtable api (they are
together with rhlist codes in __rhashtable_insert_fast).

Thanks

>
> Neil
>

Re: [PATCH net] sctp: call rcu_read_lock before checking for duplicate transport nodes

[Neil Horman]

On Tue, Feb 28, 2017 at 12:41:29PM +0800, Xin Long wrote:
> Commit cd2b70875058 ("sctp: check duplicate node before inserting a
> new transport") called rhltable_lookup() to check for the duplicate
> transport node in transport rhashtable.
> 
> But rhltable_lookup() doesn't call rcu_read_lock inside, it could cause
> a use-after-free issue if it tries to dereference the node that another
> cpu has freed it. Note that sock lock can not avoid this as it is per
> sock.
> 
> This patch is to fix it by calling rcu_read_lock before checking for
> duplicate transport nodes.
> 
> Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new 
> transport")
> Reported-by: Andrey Konovalov 
> Signed-off-by: Xin Long 
> ---
>  net/sctp/input.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index fc45896..2a28ab2 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -884,14 +884,17 @@ int sctp_hash_transport(struct sctp_transport *t)
>   arg.paddr = >ipaddr;
>   arg.lport = htons(t->asoc->base.bind_addr.port);
>  
> + rcu_read_lock();
>   list = rhltable_lookup(_transport_hashtable, ,
>  sctp_hash_params);
>  
>   rhl_for_each_entry_rcu(transport, tmp, list, node)
>   if (transport->asoc->ep == t->asoc->ep) {
> + rcu_read_unlock();
>   err = -EEXIST;
>   goto out;
>   }
> + rcu_read_unlock();
>  
>   err = rhltable_insert_key(_transport_hashtable, ,
> >node, sctp_hash_params);
> -- 
> 2.1.0
> 
> 

Hmm, rhltable_insert_key I think returns a pointer to the existing object in the
hash table if there is a collision (according to the _rhashtable_insert_fast
description).  Wouldn't it be better to eliminate the rhl_for_each_entry_rcu
entirely, and just check the returned pointer from rhltable_insert_key?

Neil

[PATCH net] sctp: call rcu_read_lock before checking for duplicate transport nodes

[Xin Long]

Commit cd2b70875058 ("sctp: check duplicate node before inserting a
new transport") called rhltable_lookup() to check for the duplicate
transport node in transport rhashtable.

But rhltable_lookup() doesn't call rcu_read_lock inside, it could cause
a use-after-free issue if it tries to dereference the node that another
cpu has freed it. Note that sock lock can not avoid this as it is per
sock.

This patch is to fix it by calling rcu_read_lock before checking for
duplicate transport nodes.

Fixes: cd2b70875058 ("sctp: check duplicate node before inserting a new 
transport")
Reported-by: Andrey Konovalov 
Signed-off-by: Xin Long 
---
 net/sctp/input.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sctp/input.c b/net/sctp/input.c
index fc45896..2a28ab2 100644
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -884,14 +884,17 @@ int sctp_hash_transport(struct sctp_transport *t)
arg.paddr = >ipaddr;
arg.lport = htons(t->asoc->base.bind_addr.port);
 
+   rcu_read_lock();
list = rhltable_lookup(_transport_hashtable, ,
   sctp_hash_params);
 
rhl_for_each_entry_rcu(transport, tmp, list, node)
if (transport->asoc->ep == t->asoc->ep) {
+   rcu_read_unlock();
err = -EEXIST;
goto out;
}
+   rcu_read_unlock();
 
err = rhltable_insert_key(_transport_hashtable, ,
  >node, sctp_hash_params);
-- 
2.1.0