Re: git: 868868f14efc - main - sctp: improve stopping of timers

> On 19. Apr 2022, at 22:02, Andrew Turner <andrew@fubar.geek.nz> wrote:
> 
> 
>> On 19 Apr 2022, at 20:31, Michael Tuexen <tuexen@FreeBSD.org> wrote:
>> 
>> The branch main has been updated by tuexen:
>> 
>> URL: https://cgit.FreeBSD.org/src/commit/?id=868868f14efcd7e127dae6e87550357c6cdb9c6d
>> 
>> 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
>> 
>>   sctp: improve stopping of timers
>> 
>>   Reported by:    syzbot+c9c70062320aaad19de7@syzkaller.appspotmail.com
>>   MFC after:      3 days
>> ---
>> sys/netinet/sctputil.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> 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 != NULL) {
>> -			SCTP_INP_DECR_REF(inp);
>> 			tmr->ep = NULL;
>> +			SCTP_INP_DECR_REF(inp);
>> 		}
> 
> 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?
> 
>> 		if (tmr->tcb != NULL) {
>> -			atomic_subtract_int(&stcb->asoc.refcnt, 1);
>> 			tmr->tcb = NULL;
>> +			atomic_subtract_int(&stcb->asoc.refcnt, 1);
>> 		}
> 
> And here
Same as above.
> 
>> 		if (tmr->net != 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 = tmr->net;
>> 			tmr->net = 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,
>> 
> 
> Andrew
>