Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
From 6e7761fd1e445b4dedecb3b90053fe5687ef285d Mon Sep 17 00:00:00 2001
From: Vivek Reddy <vkarri@nvidia.com>
Date: Fri, 19 May 2023 23:12:45 +0000
Subject: [PATCH] From 12627632ce98ebb0e64495458d84b296df84af28 Mon Sep 17
00:00:00 2001 Subject: [PATCH] zebra: Ignore route from default table and
Prevent possible crashed from use after free

a) Sonic has a host configuration process that places default routes into
the RT_TABLE_DEFAULT to allow for working without vrf's. Additionally
the sonic fpmsyncd has no idea of actual table ids are and the actual
vrf id's are passed down to it instead. As such the routes in table
RT_TABLE_DEFAULT would be passed down and cause issues in fpmsyncd
let's cut to the chase and stop this from happening.

Long term this is a real problem in that FRR does allow the thought
of using any of the 4 billion tables available to it for doing routing.
See pbr( tables in the 10k range ), `ip import-table X` and the bgp
route-map command `table X`. For where this approach is going to fall
down.

b) This patch also prevents the usage of data after it has already
been freed. Zebra on shutdown inititates through the router_shutdown
callback a table cleanup that stores on the zfpm_g->dest_q a list
of rib_dest_t's that needs to be removed from the fpm. This process
at the same time free's the dest->rnode, the dest->rnode->table
and the dest->rnode->info pointers. As such they cannot be used
when the zfpm_g->dest_q wakes up to gather data for sending
down the fpm pipe. Let's transform this problematic usage
of freed data to data we know that has not been freed yet.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
Signed-off-by: Vivek Reddy <vkarri@nvidia.com>
---
zebra/zebra_fpm_netlink.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/zebra/zebra_fpm_netlink.c b/zebra/zebra_fpm_netlink.c
index 34be9fb39..1339974e1 100644
--- a/zebra/zebra_fpm_netlink.c
+++ b/zebra/zebra_fpm_netlink.c
@@ -279,17 +279,13 @@ static int netlink_route_info_fill(struct netlink_route_info *ri, int cmd,
rib_dest_t *dest, struct route_entry *re)
{
struct nexthop *nexthop;
- struct rib_table_info *table_info =
- rib_table_info(rib_dest_table(dest));
- struct zebra_vrf *zvrf = table_info->zvrf;

memset(ri, 0, sizeof(*ri));

ri->prefix = rib_dest_prefix(dest);
ri->af = rib_dest_af(dest);

- if (zvrf && zvrf->zns)
- ri->nlmsg_pid = zvrf->zns->netlink_dplane_out.snl.nl_pid;
+ ri->nlmsg_pid = pid;

ri->nlmsg_type = cmd;
ri->rtm_table = zvrf_id(rib_dest_vrf(dest));
@@ -307,6 +303,11 @@ static int netlink_route_info_fill(struct netlink_route_info *ri, int cmd,
return 0;
}

+ if (re->table == RT_TABLE_DEFAULT) {
+ zfpm_debug("%s: Discard RT_TABLE_DEFAULT route", __func__);
+ return 0;
+ }
+
ri->rtm_protocol = netlink_proto_from_route_type(re->type);
ri->rtm_type = RTN_UNICAST;
ri->metric = &re->metric;
--
2.17.1

30 changes: 0 additions & 30 deletions src/sonic-frr/patch/0009-ignore-route-from-default-table.patch

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
From ce67e8fd10f0250c187bab529808457c74fc4b0c Mon Sep 17 00:00:00 2001
From: Donald Sharp <sharpd@nvidia.com>
Date: Thu, 24 Mar 2022 20:02:33 -0400
Subject: [PATCH] zebra: Fix use after deletion event in freebsd

In the FreeBSD code if you delete the interface
and it has no configuration, the ifp pointer will
be deleted from the system *but* zebra continues
to dereference the just freed pointer.

==58624== Invalid read of size 1
==58624== at 0x48539F3: strlcpy (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x2B0565: ifreq_set_name (ioctl.c:48)
==58624== by 0x2B0565: if_get_flags (ioctl.c:416)
==58624== by 0x2B2D9E: ifan_read (kernel_socket.c:455)
==58624== by 0x2B2D9E: kernel_read (kernel_socket.c:1403)
==58624== by 0x499F46E: thread_call (thread.c:2002)
==58624== by 0x495D2B7: frr_run (libfrr.c:1196)
==58624== by 0x2B40B8: main (main.c:471)
==58624== Address 0x6baa7f0 is 64 bytes inside a block of size 432 free'd
==58624== at 0x484ECDC: free (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x4953A64: if_delete (if.c:283)
==58624== by 0x2A93C1: if_delete_update (interface.c:874)
==58624== by 0x2B2DF3: ifan_read (kernel_socket.c:453)
==58624== by 0x2B2DF3: kernel_read (kernel_socket.c:1403)
==58624== by 0x499F46E: thread_call (thread.c:2002)
==58624== by 0x495D2B7: frr_run (libfrr.c:1196)
==58624== by 0x2B40B8: main (main.c:471)
==58624== Block was alloc'd at
==58624== at 0x4851381: calloc (in /usr/local/libexec/valgrind/vgpreload_memcheck-amd64-freebsd.so)
==58624== by 0x496A022: qcalloc (memory.c:116)
==58624== by 0x49546BC: if_new (if.c:164)
==58624== by 0x49546BC: if_create_name (if.c:218)
==58624== by 0x49546BC: if_get_by_name (if.c:603)
==58624== by 0x2B1295: ifm_read (kernel_socket.c:628)
==58624== by 0x2A7FB6: interface_list (if_sysctl.c:129)
==58624== by 0x2E99C8: zebra_ns_enable (zebra_ns.c:127)
==58624== by 0x2E99C8: zebra_ns_init (zebra_ns.c:214)
==58624== by 0x2B3FF2: main (main.c:401)
==58624==

Zebra needs to pass back whether or not the ifp pointer
was freed when if_delete_update is called and it should
then check in ifan_read as well as ifm_read that the
ifp pointer is still valid for use.

Signed-off-by: Donald Sharp <sharpd@nvidia.com>
(cherry picked from commit d0438da6b09333d2b77a9eac2e9fffbbae6e603b)
---
zebra/if_netlink.c | 4 ++--
zebra/interface.c | 5 +++--
zebra/interface.h | 2 +-
zebra/kernel_socket.c | 27 +++++++++++++++------------
zebra/zebra_netns_notify.c | 2 +-
5 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/zebra/if_netlink.c b/zebra/if_netlink.c
index 141e4074d56..5a2f6d33b18 100644
--- a/zebra/if_netlink.c
+++ b/zebra/if_netlink.c
@@ -101,7 +101,7 @@ static void set_ifindex(struct interface *ifp, ifindex_t ifi_index,
EC_LIB_INTERFACE,
"interface rename detected on up interface: index %d was renamed from %s to %s, results are uncertain!",
ifi_index, oifp->name, ifp->name);
- if_delete_update(oifp);
+ if_delete_update(&oifp);
}
}
if_set_index(ifp, ifi_index);
@@ -2031,7 +2031,7 @@ int netlink_link_change(struct nlmsghdr *h, ns_id_t ns_id, int startup)
else if (IS_ZEBRA_IF_VXLAN(ifp))
zebra_l2_vxlanif_del(ifp);

- if_delete_update(ifp);
+ if_delete_update(&ifp);

/* If VRF, delete the VRF structure itself. */
if (zif_type == ZEBRA_IF_VRF && !vrf_is_backend_netns())
diff --git a/zebra/interface.c b/zebra/interface.c
index e4e80ec4e93..cb5dc83685c 100644
--- a/zebra/interface.c
+++ b/zebra/interface.c
@@ -769,9 +769,10 @@ static void if_delete_connected(struct interface *ifp)
}

/* Handle an interface delete event */
-void if_delete_update(struct interface *ifp)
+void if_delete_update(struct interface **pifp)
{
struct zebra_if *zif;
+ struct interface *ifp = *pifp;

if (if_is_up(ifp)) {
flog_err(
@@ -834,7 +835,7 @@ void if_delete_update(struct interface *ifp)
if (IS_ZEBRA_DEBUG_KERNEL)
zlog_debug("interface %s is being deleted from the system",
ifp->name);
- if_delete(&ifp);
+ if_delete(pifp);
}
}

diff --git a/zebra/interface.h b/zebra/interface.h
index 771398b5476..571831f87da 100644
--- a/zebra/interface.h
+++ b/zebra/interface.h
@@ -474,7 +474,7 @@ extern void if_nbr_ipv6ll_to_ipv4ll_neigh_update(struct interface *ifp,
struct in6_addr *address,
int add);
extern void if_nbr_ipv6ll_to_ipv4ll_neigh_del_all(struct interface *ifp);
-extern void if_delete_update(struct interface *ifp);
+extern void if_delete_update(struct interface **ifp);
extern void if_add_update(struct interface *ifp);
extern void if_up(struct interface *);
extern void if_down(struct interface *);
diff --git a/zebra/kernel_socket.c b/zebra/kernel_socket.c
index 573db4ae5d7..d6c43dbcb06 100644
--- a/zebra/kernel_socket.c
+++ b/zebra/kernel_socket.c
@@ -450,12 +450,13 @@ static int ifan_read(struct if_announcemsghdr *ifan)
if_get_metric(ifp);
if_add_update(ifp);
} else if (ifp != NULL && ifan->ifan_what == IFAN_DEPARTURE)
- if_delete_update(ifp);
-
- if_get_flags(ifp);
- if_get_mtu(ifp);
- if_get_metric(ifp);
+ if_delete_update(&ifp);

+ if (ifp) {
+ if_get_flags(ifp);
+ if_get_mtu(ifp);
+ if_get_metric(ifp);
+ }
if (IS_ZEBRA_DEBUG_KERNEL)
zlog_debug("%s: interface %s index %d", __func__,
ifan->ifan_name, ifan->ifan_index);
@@ -722,10 +723,10 @@ int ifm_read(struct if_msghdr *ifm)
* will still behave correctly if run on a platform
* without
*/
- if_delete_update(ifp);
+ if_delete_update(&ifp);
}
#endif /* RTM_IFANNOUNCE */
- if (if_is_up(ifp)) {
+ if (ifp && if_is_up(ifp)) {
#if defined(__bsdi__)
if_kvm_get_mtu(ifp);
#else
@@ -735,14 +736,16 @@ int ifm_read(struct if_msghdr *ifm)
}
}

+ if (ifp) {
#ifdef HAVE_NET_RT_IFLIST
- ifp->stats = ifm->ifm_data;
+ ifp->stats = ifm->ifm_data;
#endif /* HAVE_NET_RT_IFLIST */
- ifp->speed = ifm->ifm_data.ifi_baudrate / 1000000;
+ ifp->speed = ifm->ifm_data.ifi_baudrate / 1000000;

- if (IS_ZEBRA_DEBUG_KERNEL)
- zlog_debug("%s: interface %s index %d", __func__, ifp->name,
- ifp->ifindex);
+ if (IS_ZEBRA_DEBUG_KERNEL)
+ zlog_debug("%s: interface %s index %d", __func__,
+ ifp->name, ifp->ifindex);
+ }

return 0;
}
diff --git a/zebra/zebra_netns_notify.c b/zebra/zebra_netns_notify.c
index 8cc7724f053..155079cfb41 100644
--- a/zebra/zebra_netns_notify.c
+++ b/zebra/zebra_netns_notify.c
@@ -179,7 +179,7 @@ static int zebra_ns_delete(char *name)
}

UNSET_FLAG(ifp->flags, IFF_UP);
- if_delete_update(ifp);
+ if_delete_update(&ifp);
}

ns = (struct ns *)vrf->ns_ctxt;
Loading