On 3/25/25 16:38, Aaron Conole via dev wrote: > Daniel Ding <[email protected]> writes: > >> From: Daniel Ding <[email protected]> >> >> In order to collect meters since the last meter action >> to the current action in the dpif_execute_helper_cb, >> the next entry is simply obtained using nl_attr_next, and >> without considering the nested nl. This will cause memory >> access to be overflowed. >> >> Gdb debug looks like this: >> >> (gdb) print a >> $6 = (const struct nlattr *) 0xaaaad175a69c >> (gdb) n >> 1206 a = nl_attr_next(a); >> (gdb) n >> 1207 } while (a != action && >> (gdb) n >> 1206 a = nl_attr_next(a); >> (gdb) print a >> $7 = (const struct nlattr *) 0xaaaad175a69c >> (gdb) print *a >> $8 = {nla_len = 0, nla_type = 0} >> (gdb) >> >> Fixes: 076caa2fb077 ("ofproto: Meter translation.") >> Signed-off-by: Daniel Ding <[email protected]> >> --- >> lib/dpif.c | 43 ++++++++++++++++--------------------------- >> 1 file changed, 16 insertions(+), 27 deletions(-) >> >> diff --git a/lib/dpif.c b/lib/dpif.c >> index d07241f1e..05dfd2919 100644 >> --- a/lib/dpif.c >> +++ b/lib/dpif.c >> @@ -1158,11 +1158,14 @@ dpif_flow_dump_next(struct dpif_flow_dump_thread >> *thread, >> return n; >> } >> >> +enum { EXECUTE_HELPER_MAX_METER = 32 }; >> + > > Could we also prevent this with something like: > > while (a != action && nl_attr_is_valid(a) && > nl_attr_type(a) != OVS_ACTION_ATTR_METER)
I think nl_attr_is_valid(a) may still be accessing the memory beyond allocated. If we want to fix the iteration this way, we should just use NL_ATTR_FOR_EACH and break at the right time. It will perform all the necessary length checks. However, as rightly pointed out, this code can't handle nested attributes and so it's not correct in general. > > I haven't done a deep review, just wondering because from a quick > glance, it seems that this change technically imposes limits on the > action list we are helping with, and then silently drops them after > that. It may be fine to limit the number of meters we will execute for one packet, 32 meters is kind of a lot for one packet. This execution path is not great to begin with. But yes, we could have a growing dynamic array or use a list instead to avoid missing some of the meters. A bigger problem here, however, is that we're accumulating those meters, but we're not removing them when we go back from the recursive execution of the clone or other nested action, so meters accumulated for one branch of execution will also be executed for the other branch, e.g. in actions:clone(meter(1),<action-with-help>),meter(2),<action-with-help> In this scenario we will accumulate and execute both meters for the last action, while only the second meter should actually be executed. > > Also, seems you could include a test case with the fix. Yes, we should have a test case for this issue. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
