From nobody Tue Apr 19 21:05:21 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 619BD11DCC8F; Tue, 19 Apr 2022 21:05:33 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from drew.franken.de (mail-n.franken.de [193.175.24.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.franken.de", Issuer "Sectigo RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4KjbtP1ZXRz4tX9; Tue, 19 Apr 2022 21:05:33 +0000 (UTC) (envelope-from tuexen@freebsd.org) Received: from smtpclient.apple (unknown [IPv6:2a02:8109:1140:c3d:457f:1c81:847f:16e9]) (Authenticated sender: macmic) by mail-n.franken.de (Postfix) with ESMTPSA id 5B99E721E280B; Tue, 19 Apr 2022 23:05:22 +0200 (CEST) Content-Type: text/plain; charset=us-ascii List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\)) Subject: Re: git: 868868f14efc - main - sctp: improve stopping of timers From: tuexen@freebsd.org In-Reply-To: <19040381-A406-49D9-BD31-92E9791C2701@fubar.geek.nz> Date: Tue, 19 Apr 2022 23:05:21 +0200 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <48314029-809E-4BE4-990C-6A058CBC6883@freebsd.org> References: <202204191931.23JJVRqX082459@gitrepo.freebsd.org> <19040381-A406-49D9-BD31-92E9791C2701@fubar.geek.nz> To: Andrew Turner X-Mailer: Apple Mail (2.3696.80.82.1.1) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on mail-n.franken.de X-Rspamd-Queue-Id: 4KjbtP1ZXRz4tX9 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-ThisMailContainsUnwantedMimeParts: N > On 19. Apr 2022, at 22:02, Andrew Turner wrote: >=20 >=20 >> On 19 Apr 2022, at 20:31, Michael Tuexen 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 >> AuthorDate: 2022-04-19 19:29:41 +0000 >> Commit: Michael Tuexen >> 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