Alexander Bluhm <alexander.bl...@gmx.net> writes:
> Hi,
>
> Next try to fix syzkaller crash
> https://syzkaller.appspot.com/bug?id=54e16dc5bce6929e14b42e2f1379f1c18f62be43
>
> Interface group names must fit into IFNAMSIZ and be unique. But
> the kernel makes the unique check before trunkating with strlcpy().
> So there can be two interfaces groups with the same name. The kif
> is created by a name lookup. The trunkated names are equal so there
> is only one kif owned by both groups. When both groups are destroyed,
> the single kif is removed twice from the RB tree.
>
> - Check length of group name before doing the unique check.
> - The empty group name was allowed. That does not make much sense.
> Does anyone use the empty interface group?
> - Use the same check in kernel and ifconfig userland.
> - ifconfig -group does not need name sanitation. The kernel will
> just report that it does not exist.
>
> ok?
OK gnezdo@
>
> bluhm
>
> Index: sys/net/if.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/net/if.c,v
> retrieving revision 1.627
> diff -u -p -r1.627 if.c
> --- sys/net/if.c 8 Feb 2021 12:30:10 -0000 1.627
> +++ sys/net/if.c 9 Feb 2021 20:47:34 -0000
> @@ -2621,9 +2621,11 @@ if_addgroup(struct ifnet *ifp, const cha
> struct ifg_list *ifgl;
> struct ifg_group *ifg = NULL;
> struct ifg_member *ifgm;
> + size_t namelen;
>
> - if (groupname[0] && groupname[strlen(groupname) - 1] >= '0' &&
> - groupname[strlen(groupname) - 1] <= '9')
> + namelen = strlen(groupname);
> + if (namelen == 0 || namelen >= IFNAMSIZ ||
> + (groupname[namelen - 1] >= '0' && groupname[namelen - 1] <= '9'))
> return (EINVAL);
>
> TAILQ_FOREACH(ifgl, &ifp->if_groups, ifgl_next)
> Index: sbin/ifconfig/ifconfig.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sbin/ifconfig/ifconfig.c,v
> retrieving revision 1.432
> diff -u -p -r1.432 ifconfig.c
> --- sbin/ifconfig/ifconfig.c 16 Jan 2021 17:44:29 -0000 1.432
> +++ sbin/ifconfig/ifconfig.c 9 Feb 2021 21:02:50 -0000
> @@ -1634,16 +1634,20 @@ void
> setifgroup(const char *group_name, int dummy)
> {
> struct ifgroupreq ifgr;
> + size_t namelen;
>
> memset(&ifgr, 0, sizeof(ifgr));
> strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
>
> - if (group_name[0] &&
> - isdigit((unsigned char)group_name[strlen(group_name) - 1]))
> + namelen = strlen(group_name);
> + if (namelen == 0)
> + errx(1, "setifgroup: group name empty");
> + if (namelen >= IFNAMSIZ)
> + errx(1, "setifgroup: group name too long");
> + if (isdigit((unsigned char)group_name[namelen - 1]))
> errx(1, "setifgroup: group names may not end in a digit");
>
> - if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
> - errx(1, "setifgroup: group name too long");
> + strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ);
> if (ioctl(sock, SIOCAIFGROUP, (caddr_t)&ifgr) == -1) {
> if (errno != EEXIST)
> err(1," SIOCAIFGROUP");
> @@ -1658,10 +1662,6 @@ unsetifgroup(const char *group_name, int
>
> memset(&ifgr, 0, sizeof(ifgr));
> strlcpy(ifgr.ifgr_name, ifname, IFNAMSIZ);
> -
> - if (group_name[0] &&
> - isdigit((unsigned char)group_name[strlen(group_name) - 1]))
> - errx(1, "unsetifgroup: group names may not end in a digit");
>
> if (strlcpy(ifgr.ifgr_group, group_name, IFNAMSIZ) >= IFNAMSIZ)
> errx(1, "unsetifgroup: group name too long");
