Commit b56b8309bc for frr
commit b56b8309bc0d166fb8023f85aab1e5fd01fb661e
Author: Alejandro Pacheco <alepacheco99@gmail.com>
Date: Wed Oct 14 16:44:36 2026 +0200
ospfd: keep the grace LSA TLV walk within the LSA
show_ospf_grace_lsa_info() does not add an unknown TLV's size to the
running total, so after an unknown TLV it keeps walking TLVs past the
end of the LSA and reads whatever follows it. The show code runs for
"show ip ospf database opaque-link" and for LSA debug dumps.
Check every grace TLV in one helper, used by both
show_ospf_grace_lsa_info() and ospf_extract_grace_lsa_fields(), that
requires the TLV to fit in the LSA (tlv_fits(), as for the other opaque
TLVs) and to be at least as long as its type's struct (the per-type
checks both loops had before). The show loop steps over unknown TLVs.
Both loops also require a whole TLV header before reading one, as ospf6d
does; received opaque LSAs are a multiple of four bytes long, so that
part only keeps the helper safe on its own.
tests/ospfd/test_ospf_grace_lsa shows a valid grace LSA and one with
unknown TLVs through show_opaque_info_detail(). Each LSA is followed in
memory by bytes the test controls, so the unknown-TLV case fails on
master without a sanitizer.
Fixes: abd5b8c72c ("ospfd: GR helper specific show commands")
Signed-off-by: Alejandro Pacheco <alepacheco99@gmail.com>
diff --git a/ospfd/ospf_gr_helper.c b/ospfd/ospf_gr_helper.c
index 790dfa17a2..c947b57a1b 100644
--- a/ospfd/ospf_gr_helper.c
+++ b/ospfd/ospf_gr_helper.c
@@ -217,6 +217,36 @@ void ospf_gr_helper_stop(void)
ospf_delete_opaque_functab(OSPF_OPAQUE_LINK_LSA, OPAQUE_TYPE_GRACE_LSA);
}
+/*
+ * Check the grace LSA TLV at offset sum of a TLV area of length bytes:
+ * the whole TLV must fit in the area and be long enough for the fields
+ * its type carries.
+ */
+static bool ospf_grace_tlv_valid(const struct tlv_header *tlvh, int sum, uint16_t length)
+{
+ uint32_t min_size;
+
+ if (!tlv_fits(tlvh, length - sum))
+ return false;
+
+ switch (ntohs(tlvh->type)) {
+ case GRACE_PERIOD_TYPE:
+ min_size = sizeof(struct grace_tlv_graceperiod);
+ break;
+ case RESTART_REASON_TYPE:
+ min_size = sizeof(struct grace_tlv_restart_reason);
+ break;
+ case RESTARTER_IP_ADDR_TYPE:
+ min_size = sizeof(struct grace_tlv_restart_addr);
+ break;
+ default:
+ min_size = TLV_HDR_SIZE;
+ break;
+ }
+
+ return TLV_SIZE(tlvh) >= min_size;
+}
+
/*
* Extracting tlv info from GRACE LSA.
*
@@ -252,11 +282,11 @@ static int ospf_extract_grace_lsa_fields(struct ospf_lsa *lsa,
length = lsa->size - OSPF_LSA_HEADER_SIZE;
- for (tlvh = TLV_HDR_TOP(lsah); sum < length && tlvh;
+ /* Iterate as long as we have at least a TLV header available */
+ for (tlvh = TLV_HDR_TOP(lsah); sum + TLV_HDR_SIZE <= length && tlvh;
tlvh = TLV_HDR_NEXT(tlvh)) {
-
- /* Check TLV len against overall LSA */
- if (sum + TLV_SIZE(tlvh) > length) {
+ /* Check TLV len against overall LSA and its type */
+ if (!ospf_grace_tlv_valid(tlvh, sum, length)) {
if (IS_DEBUG_OSPF_GR)
zlog_debug("%s: Malformed packet: Invalid TLV len:%u",
__func__, TLV_SIZE(tlvh));
@@ -265,13 +295,6 @@ static int ospf_extract_grace_lsa_fields(struct ospf_lsa *lsa,
switch (ntohs(tlvh->type)) {
case GRACE_PERIOD_TYPE:
- if (TLV_SIZE(tlvh) <
- sizeof(struct grace_tlv_graceperiod)) {
- zlog_debug("%s: Malformed packet: Invalid grace TLV len:%u",
- __func__, TLV_SIZE(tlvh));
- return OSPF_GR_FAILURE;
- }
-
grace_period = (struct grace_tlv_graceperiod *)tlvh;
*interval = ntohl(grace_period->interval);
sum += TLV_SIZE(tlvh);
@@ -282,13 +305,6 @@ static int ospf_extract_grace_lsa_fields(struct ospf_lsa *lsa,
return OSPF_GR_FAILURE;
break;
case RESTART_REASON_TYPE:
- if (TLV_SIZE(tlvh) <
- sizeof(struct grace_tlv_restart_reason)) {
- zlog_debug("%s: Malformed packet: Invalid reason TLV len:%u",
- __func__, TLV_SIZE(tlvh));
- return OSPF_GR_FAILURE;
- }
-
gr_reason = (struct grace_tlv_restart_reason *)tlvh;
*reason = gr_reason->reason;
sum += TLV_SIZE(tlvh);
@@ -297,13 +313,6 @@ static int ospf_extract_grace_lsa_fields(struct ospf_lsa *lsa,
return OSPF_GR_FAILURE;
break;
case RESTARTER_IP_ADDR_TYPE:
- if (TLV_SIZE(tlvh) <
- sizeof(struct grace_tlv_restart_addr)) {
- zlog_debug("%s: Malformed packet: Invalid addr TLV len:%u",
- __func__, TLV_SIZE(tlvh));
- return OSPF_GR_FAILURE;
- }
-
restart_addr = (struct grace_tlv_restart_addr *)tlvh;
addr->s_addr = restart_addr->addr.s_addr;
sum += TLV_SIZE(tlvh);
@@ -1021,10 +1030,10 @@ static void show_ospf_grace_lsa_info(struct vty *vty, struct json_object *json,
else
zlog_debug(" TLV info:");
- for (tlvh = TLV_HDR_TOP(lsah); sum < length && tlvh;
+ for (tlvh = TLV_HDR_TOP(lsah); sum + TLV_HDR_SIZE <= length && tlvh;
tlvh = TLV_HDR_NEXT(tlvh)) {
- /* Check TLV len */
- if (sum + TLV_SIZE(tlvh) > length) {
+ /* Check TLV len against overall LSA and its type */
+ if (!ospf_grace_tlv_valid(tlvh, sum, length)) {
if (vty)
vty_out(vty, "%% Invalid TLV length: %u\n",
TLV_SIZE(tlvh));
@@ -1036,19 +1045,6 @@ static void show_ospf_grace_lsa_info(struct vty *vty, struct json_object *json,
switch (ntohs(tlvh->type)) {
case GRACE_PERIOD_TYPE:
- if (TLV_SIZE(tlvh)
- < sizeof(struct grace_tlv_graceperiod)) {
- if (vty)
- vty_out(vty,
- "%% Invalid grace TLV length %u\n",
- TLV_SIZE(tlvh));
- else
- zlog_debug(
- "%% Invalid grace TLV length %u",
- TLV_SIZE(tlvh));
- return;
- }
-
gracePeriod = (struct grace_tlv_graceperiod *)tlvh;
sum += TLV_SIZE(tlvh);
@@ -1060,19 +1056,6 @@ static void show_ospf_grace_lsa_info(struct vty *vty, struct json_object *json,
ntohl(gracePeriod->interval));
break;
case RESTART_REASON_TYPE:
- if (TLV_SIZE(tlvh)
- < sizeof(struct grace_tlv_restart_reason)) {
- if (vty)
- vty_out(vty,
- "%% Invalid reason TLV length %u\n",
- TLV_SIZE(tlvh));
- else
- zlog_debug(
- "%% Invalid reason TLV length %u",
- TLV_SIZE(tlvh));
- return;
- }
-
grReason = (struct grace_tlv_restart_reason *)tlvh;
sum += TLV_SIZE(tlvh);
@@ -1086,19 +1069,6 @@ static void show_ospf_grace_lsa_info(struct vty *vty, struct json_object *json,
grReason->reason));
break;
case RESTARTER_IP_ADDR_TYPE:
- if (TLV_SIZE(tlvh)
- < sizeof(struct grace_tlv_restart_addr)) {
- if (vty)
- vty_out(vty,
- "%% Invalid addr TLV length %u\n",
- TLV_SIZE(tlvh));
- else
- zlog_debug(
- "%% Invalid addr TLV length %u",
- TLV_SIZE(tlvh));
- return;
- }
-
restartAddr = (struct grace_tlv_restart_addr *)tlvh;
sum += TLV_SIZE(tlvh);
@@ -1110,6 +1080,7 @@ static void show_ospf_grace_lsa_info(struct vty *vty, struct json_object *json,
&restartAddr->addr);
break;
default:
+ sum += TLV_SIZE(tlvh);
if (vty)
vty_out(vty, " Unknown TLV type %d\n",
ntohs(tlvh->type));
diff --git a/tests/ospfd/.gitignore b/tests/ospfd/.gitignore
index e764237210..ecb79adfe3 100644
--- a/tests/ospfd/.gitignore
+++ b/tests/ospfd/.gitignore
@@ -3,4 +3,5 @@ test_ospf_adj_pacing
test_ospf_lsa_examin
test_ospf_spf
test_ospf_opaque_show
+test_ospf_grace_lsa
core
diff --git a/tests/ospfd/subdir.am b/tests/ospfd/subdir.am
index c37183ae29..0f4bdabae0 100644
--- a/tests/ospfd/subdir.am
+++ b/tests/ospfd/subdir.am
@@ -35,9 +35,18 @@ tests_ospfd_test_ospf_opaque_show_CPPFLAGS = $(TESTS_CPPFLAGS)
tests_ospfd_test_ospf_opaque_show_LDADD = $(OSPFD_TEST_LDADD)
tests_ospfd_test_ospf_opaque_show_SOURCES = tests/ospfd/test_ospf_opaque_show.c
+if OSPFD
+check_PROGRAMS += tests/ospfd/test_ospf_grace_lsa
+endif
+tests_ospfd_test_ospf_grace_lsa_CFLAGS = $(TESTS_CFLAGS)
+tests_ospfd_test_ospf_grace_lsa_CPPFLAGS = $(TESTS_CPPFLAGS)
+tests_ospfd_test_ospf_grace_lsa_LDADD = $(OSPFD_TEST_LDADD)
+tests_ospfd_test_ospf_grace_lsa_SOURCES = tests/ospfd/test_ospf_grace_lsa.c
+
EXTRA_DIST += \
tests/ospfd/test_ospf_adj_pacing.py \
tests/ospfd/test_ospf_opaque_show.py \
+ tests/ospfd/test_ospf_grace_lsa.py \
tests/ospfd/test_ospf_spf.py \
tests/ospfd/test_ospf_spf.in \
tests/ospfd/test_ospf_spf.refout \
diff --git a/tests/ospfd/test_ospf_grace_lsa.c b/tests/ospfd/test_ospf_grace_lsa.c
new file mode 100644
index 0000000000..48cca62975
--- /dev/null
+++ b/tests/ospfd/test_ospf_grace_lsa.c
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Grace-LSA show checks: "show ip ospf database opaque-link" walks the
+ * grace LSA's TLVs within the LSA, stepping over unknown TLVs.
+ *
+ * Each LSA is followed in memory by bytes the test controls, so reading
+ * past its end changes the output instead of depending on what follows
+ * an allocation.
+ */
+
+#include <zebra.h>
+
+#include "memory.h"
+#include "privs.h"
+#include "frrevent.h"
+#include "buffer.h"
+#include "command.h"
+#include "vty.h"
+
+#include "ospfd/ospfd.h"
+#include "ospfd/ospf_asbr.h"
+#include "ospfd/ospf_lsa.h"
+#include "ospfd/ospf_opaque.h"
+#include "ospfd/ospf_gr.h"
+#include "ospfd/ospf_vty.h"
+
+#define TEST_VALID "grace-lsa: show a valid grace LSA"
+#define TEST_SHOW_UNKNOWN "grace-lsa: show steps over unknown TLVs"
+
+#define BODY_MAX 256
+
+struct event_loop *master;
+struct zebra_privs_t ospfd_privs;
+
+static void failed(const char *test, const char *why, const char *out)
+{
+ printf("%s\n%s\noutput:\n%s\nfailed!\n", test, why, out);
+ exit(1);
+}
+
+/*
+ * The output of "show ip ospf database opaque-link" for a grace LSA with
+ * the given body, followed in memory by the given bytes, which are not
+ * part of the LSA. The caller frees the result (MTYPE_TMP).
+ */
+static char *grace_lsa_show(const uint8_t *body, size_t len,
+ const uint8_t *after, size_t after_len)
+{
+ uint8_t buf[OSPF_LSA_HEADER_SIZE + BODY_MAX];
+ struct lsa_header *lsah = (struct lsa_header *)buf;
+ struct ospf_lsa lsa = {};
+ struct vty *vty = vty_new();
+ char *out;
+
+ assert(len + after_len <= BODY_MAX);
+ memset(buf, 0, sizeof(buf));
+ lsah->type = OSPF_OPAQUE_LINK_LSA;
+ lsah->id.s_addr = htonl(SET_OPAQUE_LSID(OPAQUE_TYPE_GRACE_LSA, 0));
+ lsah->adv_router.s_addr = htonl(0x02020202);
+ lsah->ls_seqnum = htonl(OSPF_INITIAL_SEQUENCE_NUMBER);
+ lsah->length = htons(OSPF_LSA_HEADER_SIZE + len);
+ memcpy(buf + OSPF_LSA_HEADER_SIZE, body, len);
+ if (after_len)
+ memcpy(buf + OSPF_LSA_HEADER_SIZE + len, after, after_len);
+
+ lsa.data = lsah;
+ lsa.size = OSPF_LSA_HEADER_SIZE + len;
+
+ vty->type = VTY_TERM;
+ show_opaque_info_detail(vty, &lsa, NULL);
+ out = buffer_getstr(vty->obuf);
+ vty_close(vty);
+
+ return out;
+}
+
+static void test_valid(void)
+{
+ const uint8_t body[] = {
+ 0, 1, 0, 4, 0, 0, 0, 120, /* grace period 120 */
+ 0, 2, 0, 1, 1, 0, 0, 0, /* restart reason 1 */
+ };
+ char *out = grace_lsa_show(body, sizeof(body), NULL, 0);
+
+ if (!strstr(out, "Grace period:120") || !strstr(out, "Restart reason:Software restart"))
+ failed(TEST_VALID, "grace LSA fields not shown", out);
+ XFREE(MTYPE_TMP, out);
+ printf("%s\nOK\n", TEST_VALID);
+}
+
+static void test_show_unknown(void)
+{
+ /* Unknown TLVs take room in the LSA like any other. */
+ const uint8_t body[] = {
+ 0, 9, 0, 0, /* unknown TLV, length 0 */
+ 0, 9, 0, 0, /* unknown TLV, length 0 */
+ 0, 2, 0, 1, 1, 0, 0, 0, /* restart reason 1 */
+ };
+ const uint8_t after[] = {
+ 0, 2, 0, 1, 2, 0, 0, 0, /* restart reason 2 */
+ };
+ char *out = grace_lsa_show(body, sizeof(body), after, sizeof(after));
+
+ if (!strstr(out, "Restart reason:Software restart"))
+ failed(TEST_SHOW_UNKNOWN, "restart reason in the LSA not shown", out);
+ if (strstr(out, "Software reload/upgrade"))
+ failed(TEST_SHOW_UNKNOWN, "TLV after the LSA shown", out);
+ XFREE(MTYPE_TMP, out);
+ printf("%s\nOK\n", TEST_SHOW_UNKNOWN);
+}
+
+int main(void)
+{
+ master = event_master_create(NULL);
+ cmd_init(1);
+ vty_init(master, false);
+ zlog_aux_init("NONE: ", ZLOG_DISABLED);
+
+ ospf_vty_init();
+ ospf_opaque_init();
+ ospf_gr_helper_init();
+
+ test_valid();
+ test_show_unknown();
+
+ ospf_gr_helper_stop();
+ ospf_opaque_term();
+ vty_terminate();
+ cmd_terminate();
+ event_master_free(master);
+ return 0;
+}
diff --git a/tests/ospfd/test_ospf_grace_lsa.py b/tests/ospfd/test_ospf_grace_lsa.py
new file mode 100644
index 0000000000..b499062e26
--- /dev/null
+++ b/tests/ospfd/test_ospf_grace_lsa.py
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-or-later
+import frrtest
+
+
+class TestOspfGraceLsa(frrtest.TestMultiOut):
+ program = "./test_ospf_grace_lsa"
+
+
+TestOspfGraceLsa.okfail("grace-lsa: show a valid grace LSA")
+TestOspfGraceLsa.okfail("grace-lsa: show steps over unknown TLVs")