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

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

[Pietro Borrello]

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.

We attach the stacktrace when triggering the injected
`BUG_ON(list_empty(&ep->asocs))`:

```
[  217.044169][T18237] kernel BUG at net/sctp/diag.c:364!
[  217.044845][T18237] invalid opcode: 0000 [#1] PREEMPT SMP KASAN
[  217.045681][T18237] CPU: 0 PID: 18237 Comm: syz-executor Not
tainted 6.1.0-00003-g190ee984c3e0-dirty #72
[  217.046934][T18237] Hardware name: QEMU Standard PC (i440FX +
PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
[  217.048241][T18237] RIP: 0010:sctp_sock_filter+0x1ce/0x1d0
[...]
[  217.060554][T18237] Call Trace:
[  217.061003][T18237]  <TASK>
[  217.061409][T18237]  sctp_transport_traverse_process+0x17d/0x470
[  217.062212][T18237]  ? sctp_ep_dump+0x620/0x620
[  217.062835][T18237]  ? sctp_sock_filter+0x1d0/0x1d0
[  217.063524][T18237]  ? sctp_transport_lookup_process+0x280/0x280
[  217.064330][T18237]  ? sctp_diag_get_info+0x260/0x2c0
[  217.065026][T18237]  ? sctp_for_each_endpoint+0x16f/0x200
[  217.065762][T18237]  ? sctp_diag_get_info+0x2c0/0x2c0
[  217.066435][T18237]  ? sctp_for_each_endpoint+0x1c0/0x200
[  217.067155][T18237]  sctp_diag_dump+0x2ea/0x480
[...]
[  217.093117][T18237]  do_writev+0x22d/0x460
```
---
 net/sctp/diag.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/sctp/diag.c b/net/sctp/diag.c
index a557009e9832..02831497cc9b 100644
--- a/net/sctp/diag.c
+++ b/net/sctp/diag.c
@@ -346,6 +346,9 @@ static int sctp_sock_filter(struct sctp_endpoint *ep, struct sctp_transport *tsp
 	struct sctp_association *assoc =
 		list_entry(ep->asocs.next, struct sctp_association, asocs);
 
+	if (list_empty(&ep->asocs))
+		return 0;
+
 	/* find the ep only once through the transports by this condition */
 	if (tsp->asoc != assoc)
 		return 0;

---
base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3
change-id: 20230208-sctp-filter-73453e659360

Best regards,
-- 
Pietro Borrello <borrello@diag.uniroma1.it>

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

[Xin Long]

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.

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

[Pietro Borrello]

On Wed, 8 Feb 2023 at 20:21, Xin Long <lucien.xin@gmail.com> wrote:
>
> [...]
> > 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,

Ah, thank you for clarifying! I misunderstood the comment, and read it
like "we hold the ep, otherwise ascoc can be peeled off".

> 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

Agree. This makes a lot of sense.

> 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;
>

This is a very nice suggestion, which also avoids future issues in
case assoc would be used. I'll do that in v2. Thank you!

Best regards,
Pietro