| Message ID | 20230208-sctp-filter-v1-1-84ae70d90091@diag.uniroma1.it (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] sctp: check ep asocs list before access | expand |
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.
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
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;
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(+) --- base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3 change-id: 20230208-sctp-filter-73453e659360 Best regards,