[v3,2/2] sctp: fix ASCONF list handling

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

->auto_asconf_splist is per namespace and mangled by functions like
sctp_setsockopt_auto_asconf() which doesn't guarantee any serialization.

Also, the call to inet_sk_copy_descendant() was backuping
->auto_asconf_list through the copy but was not honoring
->do_auto_asconf, which could lead to list corruption if it was
different between both sockets.

This commit thus fixes the list handling by rcu-ifying it and using
->addr_wq_lock spinlock to protect against multiple writers.

Instead of taking the lock on sctp_sock_migrate() for copying and
restoring the list values, it's preferred to avoid rewritting it by
implementing sctp_copy_descendant().

Issue was found with a test application that kept flipping sysctl
default_auto_asconf on and off, but one could trigger it by issuing
simultaneous setsockopt() calls on multiple sockets or by
creating/destroying sockets fast enough. This is only triggerable
locally.

Fixes: 9f7d653b67ae ("sctp: Add Auto-ASCONF support (core).")
Reported-by: Ji Jianwen <jiji@redhat.com>
Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
---
 include/net/netns/sctp.h   |  3 ++-
 include/net/sctp/structs.h |  4 ++++
 net/sctp/protocol.c        |  3 ++-
 net/sctp/socket.c          | 39 ++++++++++++++++++++++++++-------------
 4 files changed, 34 insertions(+), 15 deletions(-)

On Fr, 2015-06-05 at 14:08 -0300, mleitner@redhat.com wrote:
>       if (sp->do_auto_asconf) {
> +             spin_lock_bh(&sock_net(sk)->sctp.addr_wq_lock);
>               sp->do_auto_asconf = 0;
> -             list_del(&sp->auto_asconf_list);
> +             list_del_rcu(&sp->auto_asconf_list);
> +             spin_unlock_bh(&sock_net(sk)->sctp.addr_wq_lock);
>       }

This also looks a bit unsafe to me:

My proposal would be to sock_hold/sock_put the sockets when pushing them
onto the auto_asconf_list and defer the modifications on the list until
we don't need to hold socket lock anymore (in syscalls we do have a reference
anyway).

addr_wq_lock then is only used either without lock_sock at all or only
in order addr_wq_lock -> lock_sock, which does not cause any locking
ordering issues.

Bye,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Jun 08, 2015 at 10:09:50PM +0200, Hannes Frederic Sowa wrote:
> On Fr, 2015-06-05 at 14:08 -0300, mleitner@redhat.com wrote:
> >       if (sp->do_auto_asconf) {
> > +             spin_lock_bh(&sock_net(sk)->sctp.addr_wq_lock);
> >               sp->do_auto_asconf = 0;
> > -             list_del(&sp->auto_asconf_list);
> > +             list_del_rcu(&sp->auto_asconf_list);
> > +             spin_unlock_bh(&sock_net(sk)->sctp.addr_wq_lock);
> >       }
> 
> This also looks a bit unsafe to me:
> 
> My proposal would be to sock_hold/sock_put the sockets when pushing them
> onto the auto_asconf_list and defer the modifications on the list until
                                ^^^^^^^^^^^^--- you lost me here

> we don't need to hold socket lock anymore (in syscalls we do have a reference
> anyway).

Yup.. seems we have a use-after-free with this rcu usage on
auto_asconf_splist, because if the socket was destroyed by the time the
timeout handler is running, it may still see that socket and thus we
would need two additional procedures a) to take a sock_hold() when it is
inserted on that list, and release it via call_rcu() and b) to know how
to identify such dead sockets, most likely just by checking
sp->do_auto_asconf, and skip from acting on them.

Neil, WDYT?

> addr_wq_lock then is only used either without lock_sock at all or only
> in order addr_wq_lock -> lock_sock, which does not cause any locking
> ordering issues.

No because we have to update this list on sctp_destroy_sock(), which is
called with lock_sock() held. If we add the precautions above, I think
it will be fine.

Thanks,
Marcelo

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jun 09, 2015 at 12:37:21PM -0300, Marcelo Ricardo Leitner wrote:
> On Mon, Jun 08, 2015 at 10:09:50PM +0200, Hannes Frederic Sowa wrote:
> > On Fr, 2015-06-05 at 14:08 -0300, mleitner@redhat.com wrote:
> > >       if (sp->do_auto_asconf) {
> > > +             spin_lock_bh(&sock_net(sk)->sctp.addr_wq_lock);
> > >               sp->do_auto_asconf = 0;
> > > -             list_del(&sp->auto_asconf_list);
> > > +             list_del_rcu(&sp->auto_asconf_list);
> > > +             spin_unlock_bh(&sock_net(sk)->sctp.addr_wq_lock);
> > >       }
> > 
> > This also looks a bit unsafe to me:
> > 
> > My proposal would be to sock_hold/sock_put the sockets when pushing them
> > onto the auto_asconf_list and defer the modifications on the list until
>                                 ^^^^^^^^^^^^--- you lost me here
> 
> > we don't need to hold socket lock anymore (in syscalls we do have a reference
> > anyway).
> 
> Yup.. seems we have a use-after-free with this rcu usage on
> auto_asconf_splist, because if the socket was destroyed by the time the
> timeout handler is running, it may still see that socket and thus we
> would need two additional procedures a) to take a sock_hold() when it is
> inserted on that list, and release it via call_rcu() and b) to know how
> to identify such dead sockets, most likely just by checking
> sp->do_auto_asconf, and skip from acting on them.
> 
> Neil, WDYT?
> 
That seems like a reasonable approach.

> > addr_wq_lock then is only used either without lock_sock at all or only
> > in order addr_wq_lock -> lock_sock, which does not cause any locking
> > ordering issues.
> 
> No because we have to update this list on sctp_destroy_sock(), which is
> called with lock_sock() held. If we add the precautions above, I think
> it will be fine.
> 
Agreed.

> Thanks,
> Marcelo
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html