David Marchand <david.march...@redhat.com> writes: > On Tue, Nov 26, 2019 at 3:56 PM Aaron Conole <acon...@redhat.com> wrote: >> >> The service_valid call is used without properly bounds checking the >> input parameter. Almost all instances of the service_valid call are >> inside a for() loop that prevents excessive walks, but some of the >> public APIs don't bounds check and will pass invalid arguments. >> >> Prevent this by using SERVICE_GET_OR_ERR_RET where it makes sense, >> and adding a bounds check to one service_valid() use. >> >> Fixes: 8d39d3e237c2 ("service: fix race in service on app lcore function") >> Fixes: e9139a32f6e8 ("service: add function to run on app lcore") >> Fixes: e30dd31847d2 ("service: add mechanism for quiescing") >> Signed-off-by: Aaron Conole <acon...@redhat.com> >> --- >> lib/librte_eal/common/rte_service.c | 23 +++++++++++------------ >> 1 file changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/lib/librte_eal/common/rte_service.c >> b/lib/librte_eal/common/rte_service.c >> index 79235c03f8..73de7bbade 100644 >> --- a/lib/librte_eal/common/rte_service.c >> +++ b/lib/librte_eal/common/rte_service.c >> @@ -345,11 +345,12 @@ rte_service_runner_do_callback(struct >> rte_service_spec_impl *s, >> >> >> static inline int32_t >> -service_run(uint32_t i, struct core_state *cs, uint64_t service_mask) >> +service_run(uint32_t i, struct core_state *cs, uint64_t service_mask, >> + struct rte_service_spec_impl *s) >> { >> - if (!service_valid(i)) >> - return -EINVAL; >> - struct rte_service_spec_impl *s = &rte_services[i]; >> + if (!s) >> + SERVICE_VALID_GET_OR_ERR_RET(i, s, -EINVAL); >> + > > No need to check the service if we ensure that the passed index is valid. > See below.
Okay. I will document that then ;) > >> if (s->comp_runstate != RUNSTATE_RUNNING || >> s->app_runstate != RUNSTATE_RUNNING || >> !(service_mask & (UINT64_C(1) << i))) { >> @@ -383,7 +384,7 @@ rte_service_may_be_active(uint32_t id) >> int32_t lcore_count = rte_service_lcore_list(ids, RTE_MAX_LCORE); >> int i; >> >> - if (!service_valid(id)) >> + if (id >= RTE_SERVICE_NUM_MAX || !service_valid(id)) >> return -EINVAL; >> >> for (i = 0; i < lcore_count; i++) { >> @@ -397,12 +398,10 @@ rte_service_may_be_active(uint32_t id) >> int32_t >> rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t serialize_mt_unsafe) >> { >> - /* run service on calling core, using all-ones as the service mask */ >> - if (!service_valid(id)) >> - return -EINVAL; >> - >> struct core_state *cs = &lcore_states[rte_lcore_id()]; >> - struct rte_service_spec_impl *s = &rte_services[id]; >> + struct rte_service_spec_impl *s; >> + >> + SERVICE_VALID_GET_OR_ERR_RET(id, s, -EINVAL); >> >> /* Atomically add this core to the mapped cores first, then examine >> if >> * we can run the service. This avoids a race condition between >> @@ -418,7 +417,7 @@ rte_service_run_iter_on_app_lcore(uint32_t id, uint32_t >> serialize_mt_unsafe) >> return -EBUSY; >> } >> >> - int ret = service_run(id, cs, UINT64_MAX); >> + int ret = service_run(id, cs, UINT64_MAX, s); >> >> if (serialize_mt_unsafe) >> rte_atomic32_dec(&s->num_mapped_cores); >> @@ -439,7 +438,7 @@ rte_service_runner_func(void *arg) >> >> for (i = 0; i < RTE_SERVICE_NUM_MAX; i++) { >> /* return value ignored as no change to code flow */ > > if (!service_valid(idx)) > continue; > > Plus, if we add this check here, thenall loops in this file are consistent. > WDYT? Agreed - it's better. Okay.