| From foo@baz Fri Nov 2 10:04:04 CET 2018 |
| From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| Date: Tue, 16 Oct 2018 15:18:17 -0300 |
| Subject: sctp: fix race on sctp_id2asoc |
| |
| From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| |
| [ Upstream commit b336decab22158937975293aea79396525f92bb3 ] |
| |
| syzbot reported an use-after-free involving sctp_id2asoc. Dmitry Vyukov |
| helped to root cause it and it is because of reading the asoc after it |
| was freed: |
| |
| CPU 1 CPU 2 |
| (working on socket 1) (working on socket 2) |
| sctp_association_destroy |
| sctp_id2asoc |
| spin lock |
| grab the asoc from idr |
| spin unlock |
| spin lock |
| remove asoc from idr |
| spin unlock |
| free(asoc) |
| if asoc->base.sk != sk ... [*] |
| |
| This can only be hit if trying to fetch asocs from different sockets. As |
| we have a single IDR for all asocs, in all SCTP sockets, their id is |
| unique on the system. An application can try to send stuff on an id |
| that matches on another socket, and the if in [*] will protect from such |
| usage. But it didn't consider that as that asoc may belong to another |
| socket, it may be freed in parallel (read: under another socket lock). |
| |
| We fix it by moving the checks in [*] into the protected region. This |
| fixes it because the asoc cannot be freed while the lock is held. |
| |
| Reported-by: syzbot+c7dd55d7aec49d48e49a@syzkaller.appspotmail.com |
| Acked-by: Dmitry Vyukov <dvyukov@google.com> |
| Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| Acked-by: Neil Horman <nhorman@tuxdriver.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| net/sctp/socket.c | 5 ++--- |
| 1 file changed, 2 insertions(+), 3 deletions(-) |
| |
| --- a/net/sctp/socket.c |
| +++ b/net/sctp/socket.c |
| @@ -248,11 +248,10 @@ struct sctp_association *sctp_id2assoc(s |
| |
| spin_lock_bh(&sctp_assocs_id_lock); |
| asoc = (struct sctp_association *)idr_find(&sctp_assocs_id, (int)id); |
| + if (asoc && (asoc->base.sk != sk || asoc->base.dead)) |
| + asoc = NULL; |
| spin_unlock_bh(&sctp_assocs_id_lock); |
| |
| - if (!asoc || (asoc->base.sk != sk) || asoc->base.dead) |
| - return NULL; |
| - |
| return asoc; |
| } |
| |