[ovs-dev] [PATCH v2] dynamic-string: fix a crash in ds_clone()

[Ilya Maximets]

On 8/12/21 8:33 AM, Sriharsha Basavapatna via dev wrote:
> In netdev_offload_dpdk_flow_create() when an offload request fails,
> dump_flow() is called to log a warning message. The 's_tnl' string
> in flow_patterns gets initialized in vport_to_rte_tunnel() conditionally
> via ds_put_format(). If it is not initialized, it crashes later in
> dump_flow_attr()->ds_clone()->memcpy() while dereferencing this string.
> 
> To fix this, check if memory for the src string has been allocated,
> before copying it to the dst string.
> 
> Fixes: fa44a4a3ff7b ("ovn-controller: Persist desired conntrack groups.")
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna at broadcom.com>
> 
> ---
> 
> v1->v2: fix ds_clone(); ds_cstr() not needed in callers.

Thanks!  This version looks good to me.  I'd add a few more generic
words to the commit message, so it will be easier to understand the
change on older branches, but I can do that before applying the patch.

There supposed to be a separate patch for correct initialization of
s_tnl in the lib/netdev-offload-dpdk.c, as Gaetan suggested.  We need
to initialize them with DS_EMPTY_INITIALIZER.  Though it will not be
different from the actual memory initialization point of view, it's
a more correct way to work with dynamic string.  Something like this:

@@ -1324,7 +1325,11 @@ netdev_offload_dpdk_mark_rss(struct flow_patterns *patterns,
                              struct netdev *netdev,
                              uint32_t flow_mark)
 {
-    struct flow_actions actions = { .actions = NULL, .cnt = 0 };
+    struct flow_actions actions = {
+        .actions = NULL,
+        .cnt = 0,
+        .s_tnl = DS_EMPTY_INITIALIZER,
+    };
     const struct rte_flow_attr flow_attr = {
         .group = 0,
         .priority = 0,
---

And the same for other initializations of 'struct flow_actions' and
'struct flow_patterns'.  I also noticed that free_flow_patterns()
doesn't destroy the s_tnl, i.e. leaks it.  This can be fixed in the
same patch along with correct initialization.

Could you prepare this kind of patch?

Best regards, Ilya Maximets.