netdev - Re: [PATCH v3 2/2] sctp: fix ASCONF list handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150609153721.GF4049@localhost.localdomain>
Date:	Tue, 9 Jun 2015 12:37:21 -0300
From:	Marcelo Ricardo Leitner <mleitner@...hat.com>
To:	Hannes Frederic Sowa <hannes@...essinduktion.org>
Cc:	Neil Horman <nhorman@...driver.com>, netdev@...r.kernel.org,
	linux-sctp@...r.kernel.org, Daniel Borkmann <daniel@...earbox.net>,
	Vlad Yasevich <vyasevich@...il.com>,
	Michio Honda <micchie@....wide.ad.jp>
Subject: Re: [PATCH v3 2/2] sctp: fix ASCONF list handling

On Mon, Jun 08, 2015 at 10:09:50PM +0200, Hannes Frederic Sowa wrote:
> On Fr, 2015-06-05 at 14:08 -0300, mleitner@...hat.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html