On Tue, Oct 16, 2018 at 03:18:17PM -0300, Marcelo Ricardo Leitner wrote:
> 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+c7dd55d7aec49d48e...@syzkaller.appspotmail.com
> Acked-by: Dmitry Vyukov <dvyu...@google.com>
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leit...@gmail.com>
> ---
> net/sctp/socket.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index
> f73e9d38d5ba734d7ee3347e4015fd30d355bbfa..a7722f43aa69801c31409d4914c99946ee5533f5
> 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -271,11 +271,10 @@ struct sctp_association *sctp_id2assoc(struct sock *sk,
> sctp_assoc_t id)
>
> 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;
> }
>
> --
> 2.17.1
>
>
Acked-by: Neil Horman <nhor...@tuxdriver.com>
