Re: slaacd(8): prevent crash when interface disappears
On 2021-11-28 05:13 UTC, Klemens Nanni wrote: > On Thu, Nov 18, 2021 at 09:02:00AM +0100, Florian Obser wrote: >> This might be the crash kn@ was seeing once in a blue moon. > > I somewhat doubt it, since slaacd crashed on my notebook using trunk(4) > over em(4) and athn(4), none of these interface get destroyed. > > I toggle the autoconf, set IPs, etc. but once trunk0 has its interface > index, it'll stay the same until reboot. Oh, but this is not just about autoconf interfaces, it's about *all* interfaces. Look at the RTM_NEWADDR case in handle_route_message(). If it gets a NULL if_name for *any* interface it happily passes this to update_iface() -> get_flags() -> strlcpy -> boom. It doesn't yet know if it's an autoconf interface, it currently tries to figure that out. You can probably crash slaacd like this: while :; do ifconfig vether0 inet 2001:db8::23/64; ifconfig vether0 destroy; done The route socket is restricted to AF_INET6, so you need to fiddle around with IPv6 addresses... -- I'm not entirely sure you are real.
Re: slaacd(8): prevent crash when interface disappears
On Thu, Nov 18, 2021 at 09:02:00AM +0100, Florian Obser wrote: > This is split in two for easier review and I also intend to commit it > like this. > > The first diff shuffles setting of if_index around so that it's > available in all switch cases and uses it consistently instead of > ifm->ifm_index. > > That way we can copy the if_name == NULL check into each case block > preventing crashes when an interface disappears at just the wrong > moment. > > OK? > > (btw. I found this because I transmogrified slaacd into gelatod and kn@ > reported a crash while running in debug mode so I could spot what's > wrong with it. Much harder to find in slaacd...) > > commit 2880598cb424e5d889ecdbf06d9d72d777b11569 > Author: Florian Obser > Date: Wed Nov 17 20:13:55 2021 +0100 > > normalize if_index handling in route messages OK kn > commit e8882f2329db1789914358c1c6b56ddec74fc35f > Author: Florian Obser > Date: Wed Nov 17 20:17:29 2021 +0100 > > Make sure the interface still exists before updating it. > > When we get a route message, for example an address being added > (RTM_NEWADDR, but the problem exists with most of the route messages) > and the interface gets unplugged at just the right moment > if_nametoindex(3) will return NULL. We will pass NULL through > update_iface() to get_xflags() which will then crash because we > dereference the NULL pointer there. OK kn > This might be the crash kn@ was seeing once in a blue moon. I somewhat doubt it, since slaacd crashed on my notebook using trunk(4) over em(4) and athn(4), none of these interface get destroyed. I toggle the autoconf, set IPs, etc. but once trunk0 has its interface index, it'll stay the same until reboot.
Re: slaacd(8): prevent crash when interface disappears
anyone?
On 2021-11-18 09:02 +01, Florian Obser wrote:
> This is split in two for easier review and I also intend to commit it
> like this.
>
> The first diff shuffles setting of if_index around so that it's
> available in all switch cases and uses it consistently instead of
> ifm->ifm_index.
>
> That way we can copy the if_name == NULL check into each case block
> preventing crashes when an interface disappears at just the wrong
> moment.
>
> OK?
>
> (btw. I found this because I transmogrified slaacd into gelatod and kn@
> reported a crash while running in debug mode so I could spot what's
> wrong with it. Much harder to find in slaacd...)
>
> commit 2880598cb424e5d889ecdbf06d9d72d777b11569
> Author: Florian Obser
> Date: Wed Nov 17 20:13:55 2021 +0100
>
> normalize if_index handling in route messages
>
> diff --git frontend.c frontend.c
> index 72d7929c5df..29446c11e16 100644
> --- frontend.c
> +++ frontend.c
> @@ -793,30 +793,28 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> switch (rtm->rtm_type) {
> case RTM_IFINFO:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> if (if_name == NULL) {
> - log_debug("RTM_IFINFO: lost if %d", ifm->ifm_index);
> - if_index = ifm->ifm_index;
> + log_debug("RTM_IFINFO: lost if %d", if_index);
> frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
> _index, sizeof(if_index));
> remove_iface(if_index);
> + break;
> + }
> + xflags = get_xflags(if_name);
> + if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
> + IFXF_AUTOCONF6TEMP))) {
> + log_debug("RTM_IFINFO: %s(%d) no(longer) autoconf6",
> if_name,
> + if_index);
> + frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
> + 0, _index, sizeof(if_index));
> + remove_iface(if_index);
> } else {
> - xflags = get_xflags(if_name);
> - if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
> - IFXF_AUTOCONF6TEMP))) {
> - log_debug("RTM_IFINFO: %s(%d) no(longer) "
> -"autoconf6", if_name, ifm->ifm_index);
> - if_index = ifm->ifm_index;
> - frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
> - 0, _index, sizeof(if_index));
> - remove_iface(if_index);
> - } else {
> - update_iface(ifm->ifm_index, if_name);
> + update_iface(if_index, if_name);
> #ifndef SMALL
> - update_autoconf_addresses(ifm->ifm_index,
> - if_name);
> + update_autoconf_addresses(if_index, if_name);
> #endif /* SMALL */
> - }
> }
> break;
> case RTM_IFANNOUNCE:
> @@ -830,27 +828,29 @@ handle_route_message(struct rt_msghdr *rtm, struct
> sockaddr **rti_info)
> break;
> case RTM_NEWADDR:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> - log_debug("RTM_NEWADDR: %s[%u]", if_name, ifm->ifm_index);
> - update_iface(ifm->ifm_index, if_name);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> + log_debug("RTM_NEWADDR: %s[%u]", if_name, if_index);
> + update_iface(if_index, if_name);
> break;
> case RTM_DELADDR:
> ifm = (struct if_msghdr *)rtm;
> - if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
> + if_index = ifm->ifm_index;
> + if_name = if_indextoname(if_index, ifnamebuf);
> if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
> == AF_INET6) {
> - del_addr.if_index = ifm->ifm_index;
> + del_addr.if_index = if_index;
> memcpy(_addr.addr, rti_info[RTAX_IFA], sizeof(
> del_addr.addr));
> frontend_imsg_compose_engine(IMSG_DEL_ADDRESS,
> 0, 0, _addr, sizeof(del_addr));
> - log_debug("RTM_DELADDR: %s[%u]", if_name,
> - ifm->ifm_index);
> + log_debug("RTM_DELADDR: %s[%u]", if_name, if_index);
> }
> break;
> case RTM_CHGADDRATTR:
> ifm
slaacd(8): prevent crash when interface disappears
This is split in two for easier review and I also intend to commit it
like this.
The first diff shuffles setting of if_index around so that it's
available in all switch cases and uses it consistently instead of
ifm->ifm_index.
That way we can copy the if_name == NULL check into each case block
preventing crashes when an interface disappears at just the wrong
moment.
OK?
(btw. I found this because I transmogrified slaacd into gelatod and kn@
reported a crash while running in debug mode so I could spot what's
wrong with it. Much harder to find in slaacd...)
commit 2880598cb424e5d889ecdbf06d9d72d777b11569
Author: Florian Obser
Date: Wed Nov 17 20:13:55 2021 +0100
normalize if_index handling in route messages
diff --git frontend.c frontend.c
index 72d7929c5df..29446c11e16 100644
--- frontend.c
+++ frontend.c
@@ -793,30 +793,28 @@ handle_route_message(struct rt_msghdr *rtm, struct
sockaddr **rti_info)
switch (rtm->rtm_type) {
case RTM_IFINFO:
ifm = (struct if_msghdr *)rtm;
- if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
+ if_index = ifm->ifm_index;
+ if_name = if_indextoname(if_index, ifnamebuf);
if (if_name == NULL) {
- log_debug("RTM_IFINFO: lost if %d", ifm->ifm_index);
- if_index = ifm->ifm_index;
+ log_debug("RTM_IFINFO: lost if %d", if_index);
frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0, 0,
_index, sizeof(if_index));
remove_iface(if_index);
+ break;
+ }
+ xflags = get_xflags(if_name);
+ if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
+ IFXF_AUTOCONF6TEMP))) {
+ log_debug("RTM_IFINFO: %s(%d) no(longer) autoconf6",
if_name,
+ if_index);
+ frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
+ 0, _index, sizeof(if_index));
+ remove_iface(if_index);
} else {
- xflags = get_xflags(if_name);
- if (xflags == -1 || !(xflags & (IFXF_AUTOCONF6 |
- IFXF_AUTOCONF6TEMP))) {
- log_debug("RTM_IFINFO: %s(%d) no(longer) "
- "autoconf6", if_name, ifm->ifm_index);
- if_index = ifm->ifm_index;
- frontend_imsg_compose_engine(IMSG_REMOVE_IF, 0,
- 0, _index, sizeof(if_index));
- remove_iface(if_index);
- } else {
- update_iface(ifm->ifm_index, if_name);
+ update_iface(if_index, if_name);
#ifndefSMALL
- update_autoconf_addresses(ifm->ifm_index,
- if_name);
+ update_autoconf_addresses(if_index, if_name);
#endif /* SMALL */
- }
}
break;
case RTM_IFANNOUNCE:
@@ -830,27 +828,29 @@ handle_route_message(struct rt_msghdr *rtm, struct
sockaddr **rti_info)
break;
case RTM_NEWADDR:
ifm = (struct if_msghdr *)rtm;
- if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
- log_debug("RTM_NEWADDR: %s[%u]", if_name, ifm->ifm_index);
- update_iface(ifm->ifm_index, if_name);
+ if_index = ifm->ifm_index;
+ if_name = if_indextoname(if_index, ifnamebuf);
+ log_debug("RTM_NEWADDR: %s[%u]", if_name, if_index);
+ update_iface(if_index, if_name);
break;
case RTM_DELADDR:
ifm = (struct if_msghdr *)rtm;
- if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
+ if_index = ifm->ifm_index;
+ if_name = if_indextoname(if_index, ifnamebuf);
if (rtm->rtm_addrs & RTA_IFA && rti_info[RTAX_IFA]->sa_family
== AF_INET6) {
- del_addr.if_index = ifm->ifm_index;
+ del_addr.if_index = if_index;
memcpy(_addr.addr, rti_info[RTAX_IFA], sizeof(
del_addr.addr));
frontend_imsg_compose_engine(IMSG_DEL_ADDRESS,
0, 0, _addr, sizeof(del_addr));
- log_debug("RTM_DELADDR: %s[%u]", if_name,
- ifm->ifm_index);
+ log_debug("RTM_DELADDR: %s[%u]", if_name, if_index);
}
break;
case RTM_CHGADDRATTR:
ifm = (struct if_msghdr *)rtm;
- if_name = if_indextoname(ifm->ifm_index, ifnamebuf);
+ if_index
