| Message ID | 20230208-sctp-filter-v2-1-6e1f4017f326@diag.uniroma1.it (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a1221703a0f75a9d81748c516457e0fc76951496 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v2] sctp: sctp_sock_filter(): avoid list_entry() on possibly empty list | expand |
On Thu, Feb 9, 2023 at 7:13 AM Pietro Borrello <borrello@diag.uniroma1.it> wrote: > > Use list_is_first() to check whether tsp->asoc matches the first > element of ep->asocs, as the list is not guaranteed to have an entry. > > Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file") > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> > --- > Changes in v2: > - Use list_is_first() > - Link to v1: https://lore.kernel.org/r/20230208-sctp-filter-v1-1-84ae70d90091@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 | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/net/sctp/diag.c b/net/sctp/diag.c > index a557009e9832..c3d6b92dd386 100644 > --- a/net/sctp/diag.c > +++ b/net/sctp/diag.c > @@ -343,11 +343,9 @@ static int sctp_sock_filter(struct sctp_endpoint *ep, struct sctp_transport *tsp > struct sctp_comm_param *commp = p; > struct sock *sk = ep->base.sk; > const struct inet_diag_req_v2 *r = commp->r; > - 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; > > if (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family) > > --- > base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3 > change-id: 20230208-sctp-filter-73453e659360 > > Best regards, > -- > Pietro Borrello <borrello@diag.uniroma1.it> > Acked-by: Xin Long <lucien.xin@gmail.com>
On Thu, 09 Feb 2023 12:13:05 +0000 Pietro Borrello wrote: > 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))`: Thanks for the analysis, but I'll put this in for 6.2 anyway. The patch looks fairly straightforward / provably correct, and with the Fixes tag present chances are it will end up in stable either way. With a difference of maybe a week, since the merge window is just a week away..
Hello: This patch was applied to netdev/net.git (master) by Jakub Kicinski <kuba@kernel.org>: On Thu, 09 Feb 2023 12:13:05 +0000 you wrote: > Use list_is_first() to check whether tsp->asoc matches the first > element of ep->asocs, as the list is not guaranteed to have an entry. > > Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file") > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> > --- > Changes in v2: > - Use list_is_first() > - Link to v1: https://lore.kernel.org/r/20230208-sctp-filter-v1-1-84ae70d90091@diag.uniroma1.it > > [...] Here is the summary with links: - [net-next,v2] sctp: sctp_sock_filter(): avoid list_entry() on possibly empty list https://git.kernel.org/netdev/net/c/a1221703a0f7 You are awesome, thank you!
diff --git a/net/sctp/diag.c b/net/sctp/diag.c index a557009e9832..c3d6b92dd386 100644 --- a/net/sctp/diag.c +++ b/net/sctp/diag.c @@ -343,11 +343,9 @@ static int sctp_sock_filter(struct sctp_endpoint *ep, struct sctp_transport *tsp struct sctp_comm_param *commp = p; struct sock *sk = ep->base.sk; const struct inet_diag_req_v2 *r = commp->r; - 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; if (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family)
Use list_is_first() to check whether tsp->asoc matches the first element of ep->asocs, as the list is not guaranteed to have an entry. Fixes: 8f840e47f190 ("sctp: add the sctp_diag.c file") Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> --- Changes in v2: - Use list_is_first() - Link to v1: https://lore.kernel.org/r/20230208-sctp-filter-v1-1-84ae70d90091@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 | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) --- base-commit: 4ec5183ec48656cec489c49f989c508b68b518e3 change-id: 20230208-sctp-filter-73453e659360 Best regards,