From 1b870c247e70b46585751f503995208ec3229045 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Mon, 21 Oct 2024 10:45:06 +0200 Subject: [PATCH] zebra: prevent from creating pic nhe resolving over themselves There is no control on the returned nexthop group entry, when finding pic contexts. Actually the pic context can resolve over itself, and this may lead to stack overflow: The below can be found by generalizing the search of pic nhe for all nexthops and not only for srv6 contexts. > root@ubuntu2204hwe:~/frr# AddressSanitizer:DEADLYSIGNAL > ================================================================= > ==247856==ERROR: AddressSanitizer: stack-overflow on address 0x7ffe4e6dcff8 (pc 0x561e05bb5653 bp 0x7ffe4e6dd020 sp 0x7ffe4e6dd000 T0) > #0 0x561e05bb5653 in zebra_nhg_install_kernel zebra/zebra_nhg.c:3310 > #1 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #2 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #3 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #4 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #5 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #6 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #7 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #8 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #9 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #10 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #11 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #12 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #13 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 > #14 0x561e05bb572d in zebra_nhg_install_kernel zebra/zebra_nhg.c:3329 Fix this by not returning a nexthop group entry when creation is necessary for pic context. Add a check when the pic creation is not needed and the returned nhe has the same identifier as the requested nhe. Signed-off-by: Philippe Guibert --- zebra/zebra_nhg.c | 49 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c index 043f46d83eb9..1a3a59d2cc4d 100644 --- a/zebra/zebra_nhg.c +++ b/zebra/zebra_nhg.c @@ -686,7 +686,7 @@ static struct nhg_hash_entry *handle_recursive_depend(struct nhg_hash_entry *nhe depend ? depend->id : 0); if (depend) { - if (depend->id == nhe->id) { + if (pic && depend->id == nhe->id) { if (IS_ZEBRA_DEBUG_RIB_DETAILED) zlog_debug("%s: NHE %d resolved against itself", __func__, nhe->id); @@ -722,10 +722,10 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ bool created = false; bool createdPic = false; bool recursive = false; - struct nhg_hash_entry *newnhe, *backup_nhe, *pic_nhe; + struct nhg_hash_entry *newnhe, *backup_nhe, *pic_nhe = NULL, + *temp_nhe = NULL; struct nexthop *nh = NULL; - if (lookup->id) (*nhe) = zebra_nhg_lookup_id(lookup->id); else @@ -805,7 +805,14 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ if (nh->next == NULL && newnhe->id < ZEBRA_NHG_PROTO_LOWER) { if (CHECK_FLAG(nh->flags, NEXTHOP_FLAG_RECURSIVE)) { /* Single recursive nexthop */ - handle_recursive_depend(newnhe, nh->resolved, afi, pic); + temp_nhe = handle_recursive_depend(newnhe, nh->resolved, + afi, pic); + if (pic && temp_nhe == NULL) { + /* recursive dependency failed */ + zebra_nhg_free(newnhe); + *nhe = NULL; + return 0; + } recursive = true; } } else { @@ -819,10 +826,16 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ NEXTHOP_FLAG_RECURSIVE) ? "(R)" : ""); - depends_find_add(newnhe, nh, afi, from_dplane, pic); + temp_nhe = depends_find_add(newnhe, nh, afi, + from_dplane, pic); + if (pic && temp_nhe == NULL) { + /* recursive dependency failed */ + zebra_nhg_free(newnhe); + *nhe = NULL; + return 0; + } } } - if (recursive) SET_FLAG(newnhe->flags, NEXTHOP_GROUP_RECURSIVE); @@ -853,7 +866,8 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ __func__, nh); /* Single recursive nexthop */ - handle_recursive_depend(backup_nhe, nh->resolved, afi, pic); + temp_nhe = handle_recursive_depend(backup_nhe, nh->resolved, + afi, pic); recursive = true; } else { /* One or more backup NHs */ @@ -865,7 +879,14 @@ static bool zebra_nhe_find(struct nhg_hash_entry **nhe, /* return value */ NEXTHOP_FLAG_RECURSIVE) ? "(R)" : ""); - depends_find_add(backup_nhe, nh, afi, from_dplane, pic); + temp_nhe = depends_find_add(backup_nhe, nh, afi, + from_dplane, pic); + if (pic && temp_nhe == NULL) { + /* recursive dependency failed */ + zebra_nhg_free(newnhe); + *nhe = NULL; + return 0; + } } } @@ -954,6 +975,16 @@ bool zebra_pic_nhe_find(struct nhg_hash_entry **pic_nhe, /* return value */ created = zebra_nhe_find(&picnhe, &pic_nh_lookup, NULL, afi, from_dplane, true); + if (created == false && (picnhe == NULL || picnhe->id == nhe->id)) { + if (IS_ZEBRA_DEBUG_NHG_DETAIL) { + zlog_debug("%s: nhe %u failed to find a PIC NHE (recursive loop?)", + __func__, nhe->id); + *pic_nhe = NULL; + } + if (pic_nh_lookup.nhg.nexthop) + nexthops_free(pic_nh_lookup.nhg.nexthop); + return false; + } *pic_nhe = picnhe; if (pic_nh_lookup.nhg.nexthop) nexthops_free(pic_nh_lookup.nhg.nexthop); @@ -1614,7 +1645,7 @@ static struct nhg_hash_entry *depends_find_add(struct nhg_hash_entry *nhe, __func__, nh, depend); if (depend) { - if (depend->id == nhe->id) { + if (pic && depend->id == nhe->id) { if (IS_ZEBRA_DEBUG_RIB_DETAILED) zlog_debug("%s: NHE %d resolved against itself", __func__, nhe->id);