From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: interface ioctl loopback flags
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org
Date: Thu, 11 Apr 2024 16:38:46 +0200

On Thu, Apr 11, 2024 at 04:27:38PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> syzkaller has found a crash in igmp_leavegroup.
> 
> https://syzkaller.appspot.com/bug?extid=2f24ed6c8ddb2d6bb22c
> 
> uvm_fault(0xfffffd80074a6158, 0x4, 0, 1) -> e
> kernel: page fault trap, code=0
> Stopped at      igmp_leavegroup+0xaf:   movl    0x4(%rax),%r12d
>     TID    PID    UID     PRFLAGS     PFLAGS  CPU  COMMAND
> igmp_leavegroup(ffff800000dcbe00,ffff8000006ab000) at igmp_leavegroup+0xaf sys/netinet/igmp.c:512
> in_delmulti(ffff800000dcbe00) at in_delmulti+0xd3 sys/netinet/in.c:908
> ip_freemoptions(ffff8000006ba790) at ip_freemoptions+0x5d sys/netinet/ip_output.c:1737
> in_pcbdetach(fffffd806ed65168) at in_pcbdetach+0x97 sys/netinet/in_pcb.c:606
> udp_detach(fffffd806e5f4948) at udp_detach+0x3f sys/netinet/udp_usrreq.c:1139
> soclose(fffffd806e5f4948,0) at soclose+0x80 pru_detach sys/sys/protosw.h:283 [inline]
> soclose(fffffd806e5f4948,0) at soclose+0x80 sys/kern/uipc_socket.c:411
> soo_close(fffffd807251a350,ffff80002a5d71f8) at soo_close+0x44
> fdrop(fffffd807251a350,ffff80002a5d71f8) at fdrop+0xd5 sys/kern/kern_descrip.c:1274
> closef(fffffd807251a350,ffff80002a5d71f8) at closef+0x11b sys/kern/kern_descrip.c:1258
> fdfree(ffff80002a5d71f8) at fdfree+0xe3 sys/kern/kern_descrip.c:1190
> exit1(ffff80002a5d71f8,0,0,1) at exit1+0x371 sys/kern/kern_exit.c:199
> sys_exit(ffff80002a5d71f8,ffff80002a6650e0,ffff80002a665030) at sys_exit+0x1a sys/kern/kern_exit.c:89
> syscall(ffff80002a6650e0) at syscall+0x72a sys/arch/amd64/amd64/trap.c:577
> Xsyscall() at Xsyscall+0x128
> 
> It crashes here as inm->inm_rti is NULL.
> 
> void
> igmp_leavegroup(struct in_multi *inm, struct ifnet *ifp)
> {
>         switch (inm->inm_state) {
>         case IGMP_DELAYING_MEMBER:
>         case IGMP_IDLE_MEMBER:
>                 if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
>                     (ifp->if_flags & IFF_LOOPBACK) == 0)
> *                       if (inm->inm_rti->rti_type != IGMP_v1_ROUTER)
>                                 igmp_sendpkt(ifp, inm,
>                                     IGMP_HOST_LEAVE_MESSAGE,
>                                     INADDR_ALLROUTERS_GROUP);
>                 break;
>         case IGMP_LAZY_MEMBER:
>         case IGMP_AWAKENING_MEMBER:
>         case IGMP_SLEEPING_MEMBER:
>                 break;
>         }
> }
> 
> The inm->inm_rti should be set in igmp_joingroup() which calls rti_fill().
> 
> void
> igmp_joingroup(struct in_multi *inm, struct ifnet *ifp)
> {
>         int i;
> 
>         inm->inm_state = IGMP_IDLE_MEMBER;
> 
>         if (!IN_LOCAL_GROUP(inm->inm_addr.s_addr) &&
>             (ifp->if_flags & IFF_LOOPBACK) == 0) {
> *               i = rti_fill(inm);
>                 igmp_sendpkt(ifp, inm, i, 0);
>                 inm->inm_state = IGMP_DELAYING_MEMBER;
>                 inm->inm_timer = IGMP_RANDOM_DELAY(
>                     IGMP_MAX_HOST_REPORT_DELAY * PR_FASTHZ);
>                 igmp_timers_are_running = 1;
>         } else
>                 inm->inm_timer = 0;
> }
> 
> Note that both places have the condition (ifp->if_flags & IFF_LOOPBACK).
> ddb in igmp_leavegroup() shows that ifp is lo0 without IFF_LOOPBACK.
> 
> As lo0 is a loopback interface, the rti_fill() is skipped and
> inm->inm_rti is kept as NULL.  Then syzkaller clears the IFF_LOOPBACK
> flag with SIOCSIFFLAGS ioctl.  Finally igmp_leavegroup() expects
> that inm->inm_rti exists for non loopback interface.
> 
> Is there any usecase that userland can modify the IFF_LOOPBACK flag?
> If I add IFF_LOOPBACK to the unchangeable flags, kernel does not crash.
> 
> ok?

OK claudio@. I see no reason why userland should be able to toggle
IFF_LOOPBACK. The drivers don't expect that change.
 
> bluhm
> 
> Index: net/if.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.h,v
> diff -u -p -r1.215 if.h
> --- net/if.h	11 Nov 2023 14:24:03 -0000	1.215
> +++ net/if.h	11 Apr 2024 13:58:22 -0000
> @@ -219,7 +219,7 @@ struct if_status_description {
>  
>  /* flags set internally only: */
>  #define	IFF_CANTCHANGE \
> -	(IFF_BROADCAST|IFF_POINTOPOINT|IFF_RUNNING|IFF_OACTIVE|\
> +	(IFF_BROADCAST|IFF_LOOPBACK|IFF_POINTOPOINT|IFF_RUNNING|IFF_OACTIVE|\
>  	    IFF_SIMPLEX|IFF_MULTICAST|IFF_ALLMULTI)
>  
>  #define	IFXF_MPSAFE		0x1	/* [I] if_start is mpsafe */
> 

-- 
:wq Claudio