Date: Tue, 19 Apr 2022 23:05:21 +0200 From: tuexen@freebsd.org To: Andrew Turner <andrew@fubar.geek.nz> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 868868f14efc - main - sctp: improve stopping of timers Message-ID: <48314029-809E-4BE4-990C-6A058CBC6883@freebsd.org> In-Reply-To: <19040381-A406-49D9-BD31-92E9791C2701@fubar.geek.nz> References: <202204191931.23JJVRqX082459@gitrepo.freebsd.org> <19040381-A406-49D9-BD31-92E9791C2701@fubar.geek.nz>
next in thread | previous in thread | raw e-mail | index | archive | help
> On 19. Apr 2022, at 22:02, Andrew Turner <andrew@fubar.geek.nz> wrote: >=20 >=20 >> On 19 Apr 2022, at 20:31, Michael Tuexen <tuexen@FreeBSD.org> wrote: >>=20 >> The branch main has been updated by tuexen: >>=20 >> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D868868f14efcd7e127dae6e87550357c= 6cdb9c6d >>=20 >> commit 868868f14efcd7e127dae6e87550357c6cdb9c6d >> Author: Michael Tuexen <tuexen@FreeBSD.org> >> AuthorDate: 2022-04-19 19:29:41 +0000 >> Commit: Michael Tuexen <tuexen@FreeBSD.org> >> CommitDate: 2022-04-19 19:29:41 +0000 >>=20 >> sctp: improve stopping of timers >>=20 >> Reported by: = syzbot+c9c70062320aaad19de7@syzkaller.appspotmail.com >> MFC after: 3 days >> --- >> sys/netinet/sctputil.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >>=20 >> diff --git a/sys/netinet/sctputil.c b/sys/netinet/sctputil.c >> index 8c96a832827a..49a8abbc9ccf 100644 >> --- a/sys/netinet/sctputil.c >> +++ b/sys/netinet/sctputil.c >> @@ -2869,20 +2869,23 @@ sctp_timer_stop(int t_type, struct sctp_inpcb = *inp, struct sctp_tcb *stcb, >> * counts that were incremented in sctp_timer_start(). >> */ >> if (tmr->ep !=3D NULL) { >> - SCTP_INP_DECR_REF(inp); >> tmr->ep =3D NULL; >> + SCTP_INP_DECR_REF(inp); >> } >=20 > It looks like SCTP_INP_DECR_REF and setting tmr->ep could still be = reordered on architectures with weak memory ordering. I don't think that is a problem here. I just clear the pointer. I = changed the sequence in the code to do it consistently. Do you think this is a problem? >=20 >> if (tmr->tcb !=3D NULL) { >> - atomic_subtract_int(&stcb->asoc.refcnt, 1); >> tmr->tcb =3D NULL; >> + atomic_subtract_int(&stcb->asoc.refcnt, 1); >> } >=20 > And here Same as above. >=20 >> if (tmr->net !=3D NULL) { >> + struct sctp_nets *tmr_net; >> + >> /* >> * Can't use net, since it doesn't work for >> * SCTP_TIMER_TYPE_ASCONF. >> */ >> - sctp_free_remote_addr((struct sctp_nets = *)tmr->net); >> + tmr_net =3D tmr->net; >> tmr->net =3D NULL; >> + sctp_free_remote_addr((struct sctp_nets = *)tmr_net); Here is the critical part of the patch. sctp_free_remote_addr() can = result in freeing the net, and for some timers, the timer is part of the net. So this code would set the = net component of the just freed timer. I think this is what the syzkaller issue (a UAF) is = about. Best regards Michael >> } >> } else { >> SCTPDBG(SCTP_DEBUG_TIMER2, >>=20 >=20 > Andrew >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?48314029-809E-4BE4-990C-6A058CBC6883>