Re: [PATCH net-next] sctp: check ep asocs list before access

On Wed, Feb 8, 2023 at 1:13 PM Pietro Borrello
[off-list ref] 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 <redacted>
---

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.