From 755b529c11d37f839dfba91127657a47390b88a2 Mon Sep 17 00:00:00 2001 From: Jon Loeliger Date: Mon, 12 Sep 2022 12:41:06 -0500 Subject: abf: add API parameter n_paths range checks Also check for non-zero rpath length in CLI cmd. While there, no need to use "else" after a return. Also while there, notice and fix numerous input_line buffer leaks and fix them. Type: fix Fixes: 669d07dc016757b856e1014a415996cf9f0ebc58 Signed-off-by: Jon Loeliger Change-Id: I18ea44b7b82e8938c3e793e7c2a04dfe157076d8 --- src/plugins/abf/abf.api | 2 +- src/plugins/abf/abf_api.c | 6 +++ src/plugins/abf/abf_policy.c | 91 ++++++++++++++++++++++++-------------------- 3 files changed, 56 insertions(+), 43 deletions(-) (limited to 'src/plugins/abf') diff --git a/src/plugins/abf/abf.api b/src/plugins/abf/abf.api index 1cd3da7e557..a748de4522b 100644 --- a/src/plugins/abf/abf.api +++ b/src/plugins/abf/abf.api @@ -51,7 +51,7 @@ define abf_plugin_get_version_reply /** \brief A description of an ABF policy @param policy_id User chosen Identifier for the policy @param acl_index The ACL that the policy will match against - @param n_paths Number of paths + @param n_paths Number of paths, 1..255 @param paths The set of forwarding paths that are being added or removed. */ typedef abf_policy diff --git a/src/plugins/abf/abf_api.c b/src/plugins/abf/abf_api.c index 66121acbdf0..222e1f4e116 100644 --- a/src/plugins/abf/abf_api.c +++ b/src/plugins/abf/abf_api.c @@ -69,6 +69,12 @@ vl_api_abf_policy_add_del_t_handler (vl_api_abf_policy_add_del_t * mp) int rv = 0; u8 pi; + if (mp->policy.n_paths == 0) + { + rv = VNET_API_ERROR_INVALID_VALUE; + goto done; + } + vec_validate (paths, mp->policy.n_paths - 1); for (pi = 0; pi < mp->policy.n_paths; pi++) diff --git a/src/plugins/abf/abf_policy.c b/src/plugins/abf/abf_policy.c index 1921df113ff..8f0fc116144 100644 --- a/src/plugins/abf/abf_policy.c +++ b/src/plugins/abf/abf_policy.c @@ -192,50 +192,45 @@ abf_policy_delete (u32 policy_id, const fib_route_path_t * rpaths) */ return (VNET_API_ERROR_INVALID_VALUE); } - else - { - /* - * update an existing policy. - * - add the path to the path-list and swap our ancestry - * - backwalk to poke all attachments to update - */ - fib_node_index_t old_pl; - ap = abf_policy_get (api); - old_pl = ap->ap_pl; + /* + * update an existing policy. + * - add the path to the path-list and swap our ancestry + * - backwalk to poke all attachments to update + */ + fib_node_index_t old_pl; - fib_path_list_lock (old_pl); - ap->ap_pl = - fib_path_list_copy_and_path_remove (ap->ap_pl, - (FIB_PATH_LIST_FLAG_SHARED | - FIB_PATH_LIST_FLAG_NO_URPF), - rpaths); + ap = abf_policy_get (api); + old_pl = ap->ap_pl; - fib_path_list_child_remove (old_pl, ap->ap_sibling); - ap->ap_sibling = ~0; + fib_path_list_lock (old_pl); + ap->ap_pl = fib_path_list_copy_and_path_remove ( + ap->ap_pl, (FIB_PATH_LIST_FLAG_SHARED | FIB_PATH_LIST_FLAG_NO_URPF), + rpaths); - if (FIB_NODE_INDEX_INVALID == ap->ap_pl) - { - /* - * no more paths on this policy. It's toast - * remove the CLI/API's lock - */ - fib_node_unlock (&ap->ap_node); - } - else - { - ap->ap_sibling = fib_path_list_child_add (ap->ap_pl, - abf_policy_fib_node_type, - api); + fib_path_list_child_remove (old_pl, ap->ap_sibling); + ap->ap_sibling = ~0; - fib_node_back_walk_ctx_t ctx = { - .fnbw_reason = FIB_NODE_BW_REASON_FLAG_EVALUATE, - }; + if (FIB_NODE_INDEX_INVALID == ap->ap_pl) + { + /* + * no more paths on this policy. It's toast + * remove the CLI/API's lock + */ + fib_node_unlock (&ap->ap_node); + } + else + { + ap->ap_sibling = + fib_path_list_child_add (ap->ap_pl, abf_policy_fib_node_type, api); - fib_walk_sync (abf_policy_fib_node_type, api, &ctx); - } - fib_path_list_unlock (old_pl); + fib_node_back_walk_ctx_t ctx = { + .fnbw_reason = FIB_NODE_BW_REASON_FLAG_EVALUATE, + }; + + fib_walk_sync (abf_policy_fib_node_type, api, &ctx); } + fib_path_list_unlock (old_pl); return (0); } @@ -272,14 +267,25 @@ abf_policy_cmd (vlib_main_t * vm, unformat_fib_route_path, &rpath, &payload_proto)) vec_add1 (rpaths, rpath); else - return (clib_error_return (0, "unknown input '%U'", - format_unformat_error, line_input)); + { + clib_error_t *err; + err = clib_error_return (0, "unknown input '%U'", + format_unformat_error, line_input); + unformat_free (line_input); + return err; + } } if (INDEX_INVALID == policy_id) { vlib_cli_output (vm, "Specify a Policy ID"); - return 0; + goto out; + } + + if (vec_len (rpaths) == 0) + { + vlib_cli_output (vm, "Hop path must not be empty"); + goto out; } if (!is_del) @@ -287,7 +293,7 @@ abf_policy_cmd (vlib_main_t * vm, if (INDEX_INVALID == acl_index) { vlib_cli_output (vm, "ACL index must be set"); - return 0; + goto out; } rv = abf_policy_update (policy_id, acl_index, rpaths); @@ -296,7 +302,7 @@ abf_policy_cmd (vlib_main_t * vm, { vlib_cli_output (vm, "ACL index must match existing ACL index in policy"); - return 0; + goto out; } } else @@ -304,6 +310,7 @@ abf_policy_cmd (vlib_main_t * vm, abf_policy_delete (policy_id, rpaths); } +out: unformat_free (line_input); return (NULL); } -- cgit 1.2.3-korg