MIME-Version: 1.0
References: <20230208-sctp-filter-v1-1-84ae70d90091@diag.uniroma1.it>
In-Reply-To: <20230208-sctp-filter-v1-1-84ae70d90091@diag.uniroma1.it>
From:   Xin Long <lucien.xin@gmail.com>
Date:   Wed, 8 Feb 2023 14:20:39 -0500
Message-ID: <CADvbK_ebZEmO_n9c3XDBF65W8AcXFXdUYjpsRDUin8T0devCYQ@mail.gmail.com>
Subject: Re: [PATCH net-next] sctp: check ep asocs list before access
To:     Pietro Borrello <borrello@diag.uniroma1.it>
Cc:     Neil Horman <nhorman@tuxdriver.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Cristiano Giuffrida <c.giuffrida@vu.nl>,
        "Bos, H.J." <h.j.bos@vu.nl>, Jakob Koschel <jkl820.git@gmail.com>,
        linux-sctp@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Feb 8, 2023 at 1:13 PM Pietro Borrello
<borrello@diag.uniroma1.it> wrote:
>
> Add list_empty() check before accessing first entry of ep->asocs list
> in sctp_sock_filter(), which is not gauranteed to have an entry.
>
> Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file")
> Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> ---
>
> The list_entry on an empty list creates a type confused pointer.
> While using it is undefined behavior, in this case it seems there
> is no big risk, as the `tsp->asoc != assoc` check will almost
> certainly fail on the type confused pointer.
> We report this bug also since it may hide further problems since
> the code seems to assume a non-empty `ep->asocs`.
>
> We were able to trigger sctp_sock_filter() using syzkaller, and
> cause a panic inserting `BUG_ON(list_empty(&ep->asocs))`, so the
> list may actually be empty.
> But we were not able to minimize our testcase and understand how
> sctp_sock_filter may end up with an empty asocs list.
> We suspect a race condition between a connecting sctp socket
> and the diag query.
As it commented in sctp_transport_traverse_process():

"asoc can be peeled off " before callinsctp_sock_filter(). Actually,
the asoc can be peeled off from the ep anytime during it by another
thread, and placing a list_empty(&ep->asocs) check and returning
won't avoid it completely, as peeling off the asoc can happen after
your check.

We actually don't care about the asoc peeling off during the dump,
as sctp diag can not work that accurately. There also shouldn't be
problems caused so far, as the "assoc" won't be used anywhere after
that check.

To avoid the "type confused pointer" thing,  maybe you can try to use
list_is_first() there:

-       struct sctp_association *assoc =
-               list_entry(ep->asocs.next, struct sctp_association, asocs);

        /* find the ep only once through the transports by this condition */
-       if (tsp->asoc != assoc)
+       if (!list_is_first(&tsp->asoc->asocs, &ep->asocs))
                return 0;

Thanks.