From 269b69d70336f8f831e761ce7231e4574477e6c0 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 4 May 2021 23:43:47 -0400 Subject: [PATCH 1/2] zebra: memset the `struct rtattr *tb[SIZE]` in setting function In order to parse the netlink message into the `struct rtattr *tb[size]` it is assumed that the buffer is memset to 0 before the parsing. As such if you attempt to read a value that was not returned in the message you will not crash when you test for it. The code has places were we memset it and places where we don't. This *will* lead to crashes when the kernel changes. In our parsing routines let's have them memset instead of having to remember to do it pre pass in to the parser. Signed-off-by: Donald Sharp --- zebra/if_netlink.c | 13 +------------ zebra/kernel_netlink.c | 2 ++ zebra/rt_netlink.c | 7 ------- 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/zebra/if_netlink.c b/zebra/if_netlink.c index fbf64439e32c..2b28c2591054 100644 --- a/zebra/if_netlink.c +++ b/zebra/if_netlink.c @@ -312,7 +312,6 @@ static void netlink_vrf_change(struct nlmsghdr *h, struct rtattr *tb, ifi = NLMSG_DATA(h); - memset(linkinfo, 0, sizeof(linkinfo)); parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, tb); if (!linkinfo[IFLA_INFO_DATA]) { @@ -323,7 +322,6 @@ static void netlink_vrf_change(struct nlmsghdr *h, struct rtattr *tb, return; } - memset(attr, 0, sizeof(attr)); parse_rtattr_nested(attr, IFLA_VRF_MAX, linkinfo[IFLA_INFO_DATA]); if (!attr[IFLA_VRF_TABLE]) { if (IS_ZEBRA_DEBUG_KERNEL) @@ -544,7 +542,6 @@ static int netlink_extract_bridge_info(struct rtattr *link_data, struct rtattr *attr[IFLA_BR_MAX + 1]; memset(bridge_info, 0, sizeof(*bridge_info)); - memset(attr, 0, sizeof(attr)); parse_rtattr_nested(attr, IFLA_BR_MAX, link_data); if (attr[IFLA_BR_VLAN_FILTERING]) bridge_info->vlan_aware = @@ -559,7 +556,6 @@ static int netlink_extract_vlan_info(struct rtattr *link_data, vlanid_t vid_in_msg; memset(vlan_info, 0, sizeof(*vlan_info)); - memset(attr, 0, sizeof(attr)); parse_rtattr_nested(attr, IFLA_VLAN_MAX, link_data); if (!attr[IFLA_VLAN_ID]) { if (IS_ZEBRA_DEBUG_KERNEL) @@ -579,7 +575,7 @@ static int netlink_extract_gre_info(struct rtattr *link_data, memset(gre_info, 0, sizeof(*gre_info)); memset(attr, 0, sizeof(attr)); - parse_rtattr_nested(attr, IFLA_GRE_MAX, link_data); + netlink_parse_rtattr_nested(attr, IFLA_GRE_MAX, link_data); if (!attr[IFLA_GRE_LOCAL]) { if (IS_ZEBRA_DEBUG_KERNEL) @@ -622,7 +618,6 @@ static int netlink_extract_vxlan_info(struct rtattr *link_data, ifindex_t ifindex_link; memset(vxl_info, 0, sizeof(*vxl_info)); - memset(attr, 0, sizeof(attr)); parse_rtattr_nested(attr, IFLA_VXLAN_MAX, link_data); if (!attr[IFLA_VXLAN_ID]) { if (IS_ZEBRA_DEBUG_KERNEL) @@ -716,7 +711,6 @@ static int netlink_bridge_vxlan_update(struct interface *ifp, /* There is a 1-to-1 mapping of VLAN to VxLAN - hence * only 1 access VLAN is accepted. */ - memset(aftb, 0, sizeof(aftb)); parse_rtattr_nested(aftb, IFLA_BRIDGE_MAX, af_spec); if (!aftb[IFLA_BRIDGE_VLAN_INFO]) return 0; @@ -786,7 +780,6 @@ static int netlink_bridge_interface(struct nlmsghdr *h, int len, ns_id_t ns_id, /* Fetch name and ifindex */ ifi = NLMSG_DATA(h); - memset(tb, 0, sizeof(tb)); netlink_parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len); if (tb[IFLA_IFNAME] == NULL) @@ -854,7 +847,6 @@ static uint8_t netlink_parse_lacp_bypass(struct rtattr **linkinfo) uint8_t bypass = 0; struct rtattr *mbrinfo[IFLA_BOND_SLAVE_MAX + 1]; - memset(mbrinfo, 0, sizeof(mbrinfo)); parse_rtattr_nested(mbrinfo, IFLA_BOND_SLAVE_MAX, linkinfo[IFLA_INFO_SLAVE_DATA]); if (mbrinfo[IFLA_BOND_SLAVE_AD_RX_BYPASS]) @@ -910,7 +902,6 @@ static int netlink_interface(struct nlmsghdr *h, ns_id_t ns_id, int startup) return netlink_bridge_interface(h, len, ns_id, startup); /* Looking up interface name. */ - memset(tb, 0, sizeof(tb)); memset(linkinfo, 0, sizeof(linkinfo)); netlink_parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len); @@ -1303,7 +1294,6 @@ int netlink_interface_addr(struct nlmsghdr *h, ns_id_t ns_id, int startup) return -1; } - memset(tb, 0, sizeof(tb)); netlink_parse_rtattr(tb, IFA_MAX, IFA_RTA(ifa), len); ifp = if_lookup_by_index_per_ns(zns, ifa->ifa_index); @@ -1519,7 +1509,6 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) return netlink_bridge_interface(h, len, ns_id, startup); /* Looking up interface name. */ - memset(tb, 0, sizeof(tb)); memset(linkinfo, 0, sizeof(linkinfo)); netlink_parse_rtattr(tb, IFLA_MAX, IFLA_RTA(ifi), len); diff --git a/zebra/kernel_netlink.c b/zebra/kernel_netlink.c index cd22e95737b1..8b631a372668 100644 --- a/zebra/kernel_netlink.c +++ b/zebra/kernel_netlink.c @@ -493,6 +493,7 @@ void netlink_parse_rtattr_flags(struct rtattr **tb, int max, { unsigned short type; + memset(tb, 0, sizeof(struct rtattr *) * (max + 1)); while (RTA_OK(rta, len)) { type = rta->rta_type & ~flags; if ((type <= max) && (!tb[type])) @@ -504,6 +505,7 @@ void netlink_parse_rtattr_flags(struct rtattr **tb, int max, void netlink_parse_rtattr(struct rtattr **tb, int max, struct rtattr *rta, int len) { + memset(tb, 0, sizeof(struct rtattr *) * (max + 1)); while (RTA_OK(rta, len)) { if (rta->rta_type <= max) tb[rta->rta_type] = rta; diff --git a/zebra/rt_netlink.c b/zebra/rt_netlink.c index d2ec7da57cc1..fbf37230c731 100644 --- a/zebra/rt_netlink.c +++ b/zebra/rt_netlink.c @@ -509,8 +509,6 @@ static uint8_t parse_multipath_nexthops_unicast(ns_id_t ns_id, nh_vrf_id = vrf_id; if (rtnh->rtnh_len > sizeof(*rtnh)) { - memset(rtnh_tb, 0, sizeof(rtnh_tb)); - netlink_parse_rtattr(rtnh_tb, RTA_MAX, RTNH_DATA(rtnh), rtnh->rtnh_len - sizeof(*rtnh)); if (rtnh_tb[RTA_GATEWAY]) @@ -628,7 +626,6 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, return -1; } - memset(tb, 0, sizeof(tb)); netlink_parse_rtattr(tb, RTA_MAX, RTM_RTA(rtm), len); if (rtm->rtm_flags & RTM_F_CLONED) @@ -713,7 +710,6 @@ static int netlink_route_change_read_unicast(struct nlmsghdr *h, ns_id_t ns_id, if (tb[RTA_METRICS]) { struct rtattr *mxrta[RTAX_MAX + 1]; - memset(mxrta, 0, sizeof(mxrta)); netlink_parse_rtattr(mxrta, RTAX_MAX, RTA_DATA(tb[RTA_METRICS]), RTA_PAYLOAD(tb[RTA_METRICS])); @@ -920,7 +916,6 @@ static int netlink_route_change_read_multicast(struct nlmsghdr *h, len = h->nlmsg_len - NLMSG_LENGTH(sizeof(struct rtmsg)); - memset(tb, 0, sizeof(tb)); netlink_parse_rtattr(tb, RTA_MAX, RTM_RTA(rtm), len); if (tb[RTA_TABLE]) @@ -2875,7 +2870,6 @@ static int netlink_macfdb_change(struct nlmsghdr *h, int len, ns_id_t ns_id) /* Parse attributes and extract fields of interest. Do basic * validation of the fields. */ - memset(tb, 0, sizeof tb); netlink_parse_rtattr_flags(tb, NDA_MAX, NDA_RTA(ndm), len, NLA_F_NESTED); @@ -3348,7 +3342,6 @@ static int netlink_ipneigh_change(struct nlmsghdr *h, int len, ns_id_t ns_id) zif = (struct zebra_if *)ifp->info; /* Parse attributes and extract fields of interest. */ - memset(tb, 0, sizeof(tb)); netlink_parse_rtattr(tb, NDA_MAX, NDA_RTA(ndm), len); if (!tb[NDA_DST]) { From c9d842c710b273fe989d477ceaea3926a168cddb Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 4 May 2021 23:48:17 -0400 Subject: [PATCH 2/2] zebra: Consolidate on 1 function netlink_parse_rattr_nested if_netlink.c created it's on nested parsing #define which is identical to netlink_parse_rtattr_nested. Consolidate on one instead of having this duality. Signed-off-by: Donald Sharp --- zebra/if_netlink.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/zebra/if_netlink.c b/zebra/if_netlink.c index 2b28c2591054..c2b4dcc52f8f 100644 --- a/zebra/if_netlink.c +++ b/zebra/if_netlink.c @@ -297,9 +297,6 @@ static void netlink_determine_zebra_iftype(const char *kind, *zif_type = ZEBRA_IF_GRE; } -#define parse_rtattr_nested(tb, max, rta) \ - netlink_parse_rtattr((tb), (max), RTA_DATA(rta), RTA_PAYLOAD(rta)) - static void netlink_vrf_change(struct nlmsghdr *h, struct rtattr *tb, uint32_t ns_id, const char *name) { @@ -312,7 +309,7 @@ static void netlink_vrf_change(struct nlmsghdr *h, struct rtattr *tb, ifi = NLMSG_DATA(h); - parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, tb); + netlink_parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, tb); if (!linkinfo[IFLA_INFO_DATA]) { if (IS_ZEBRA_DEBUG_KERNEL) @@ -322,7 +319,8 @@ static void netlink_vrf_change(struct nlmsghdr *h, struct rtattr *tb, return; } - parse_rtattr_nested(attr, IFLA_VRF_MAX, linkinfo[IFLA_INFO_DATA]); + netlink_parse_rtattr_nested(attr, IFLA_VRF_MAX, + linkinfo[IFLA_INFO_DATA]); if (!attr[IFLA_VRF_TABLE]) { if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug( @@ -542,7 +540,7 @@ static int netlink_extract_bridge_info(struct rtattr *link_data, struct rtattr *attr[IFLA_BR_MAX + 1]; memset(bridge_info, 0, sizeof(*bridge_info)); - parse_rtattr_nested(attr, IFLA_BR_MAX, link_data); + netlink_parse_rtattr_nested(attr, IFLA_BR_MAX, link_data); if (attr[IFLA_BR_VLAN_FILTERING]) bridge_info->vlan_aware = *(uint8_t *)RTA_DATA(attr[IFLA_BR_VLAN_FILTERING]); @@ -556,7 +554,7 @@ static int netlink_extract_vlan_info(struct rtattr *link_data, vlanid_t vid_in_msg; memset(vlan_info, 0, sizeof(*vlan_info)); - parse_rtattr_nested(attr, IFLA_VLAN_MAX, link_data); + netlink_parse_rtattr_nested(attr, IFLA_VLAN_MAX, link_data); if (!attr[IFLA_VLAN_ID]) { if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug("IFLA_VLAN_ID missing from VLAN IF message"); @@ -618,7 +616,7 @@ static int netlink_extract_vxlan_info(struct rtattr *link_data, ifindex_t ifindex_link; memset(vxl_info, 0, sizeof(*vxl_info)); - parse_rtattr_nested(attr, IFLA_VXLAN_MAX, link_data); + netlink_parse_rtattr_nested(attr, IFLA_VXLAN_MAX, link_data); if (!attr[IFLA_VXLAN_ID]) { if (IS_ZEBRA_DEBUG_KERNEL) zlog_debug( @@ -711,7 +709,7 @@ static int netlink_bridge_vxlan_update(struct interface *ifp, /* There is a 1-to-1 mapping of VLAN to VxLAN - hence * only 1 access VLAN is accepted. */ - parse_rtattr_nested(aftb, IFLA_BRIDGE_MAX, af_spec); + netlink_parse_rtattr_nested(aftb, IFLA_BRIDGE_MAX, af_spec); if (!aftb[IFLA_BRIDGE_VLAN_INFO]) return 0; @@ -847,8 +845,8 @@ static uint8_t netlink_parse_lacp_bypass(struct rtattr **linkinfo) uint8_t bypass = 0; struct rtattr *mbrinfo[IFLA_BOND_SLAVE_MAX + 1]; - parse_rtattr_nested(mbrinfo, IFLA_BOND_SLAVE_MAX, - linkinfo[IFLA_INFO_SLAVE_DATA]); + netlink_parse_rtattr_nested(mbrinfo, IFLA_BOND_SLAVE_MAX, + linkinfo[IFLA_INFO_SLAVE_DATA]); if (mbrinfo[IFLA_BOND_SLAVE_AD_RX_BYPASS]) bypass = *(uint8_t *)RTA_DATA( mbrinfo[IFLA_BOND_SLAVE_AD_RX_BYPASS]); @@ -921,7 +919,8 @@ static int netlink_interface(struct nlmsghdr *h, ns_id_t ns_id, int startup) desc = (char *)RTA_DATA(tb[IFLA_IFALIAS]); if (tb[IFLA_LINKINFO]) { - parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, tb[IFLA_LINKINFO]); + netlink_parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, + tb[IFLA_LINKINFO]); if (linkinfo[IFLA_INFO_KIND]) kind = RTA_DATA(linkinfo[IFLA_INFO_KIND]); @@ -1525,7 +1524,8 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup) name = (char *)RTA_DATA(tb[IFLA_IFNAME]); if (tb[IFLA_LINKINFO]) { - parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, tb[IFLA_LINKINFO]); + netlink_parse_rtattr_nested(linkinfo, IFLA_INFO_MAX, + tb[IFLA_LINKINFO]); if (linkinfo[IFLA_INFO_KIND]) kind = RTA_DATA(linkinfo[IFLA_INFO_KIND]);