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. > 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? -- David Marchand