netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list
@ 2015-02-17 14:03 Alvaro Neira Ayuso
  2015-02-17 14:26 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 4+ messages in thread
From: Alvaro Neira Ayuso @ 2015-02-17 14:03 UTC (permalink / raw)
  To: netfilter-devel

When we parse a ruleset which has a rule using a set. First step is parse the
set, set up an id and add it to a set list. Later, we use this set list to find
the set associated to the rule and we set up the set id to the expression
(lookup expression) of the rule.

The problem is if we return this set using the function
nft_ruleset_parse_file_cb and we free this set. We have a crash when we try to
iterate in the set list.

This patch solves it, cloning the set and adding the new set to the set list.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[changes in v2]
 * Added the function to clone set and set elems.

 include/libnftnl/set.h |    2 ++
 src/ruleset.c          |    8 +++++++-
 src/set.c              |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/set_elem.c         |   27 +++++++++++++++++++++++++
 4 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
index 7f3504f..3f8ef87 100644
--- a/include/libnftnl/set.h
+++ b/include/libnftnl/set.h
@@ -42,6 +42,7 @@ const void *nft_set_attr_get_data(struct nft_set *s, uint16_t attr,
 				  uint32_t *data_len);
 const char *nft_set_attr_get_str(struct nft_set *s, uint16_t attr);
 uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr);
+struct nft_set *nft_set_clone(const struct nft_set *set);
 
 struct nlmsghdr;
 
@@ -103,6 +104,7 @@ const char *nft_set_elem_attr_get_str(struct nft_set_elem *s, uint16_t attr);
 uint32_t nft_set_elem_attr_get_u32(struct nft_set_elem *s, uint16_t attr);
 
 bool nft_set_elem_attr_is_set(const struct nft_set_elem *s, uint16_t attr);
+struct nft_set_elem *nft_set_elem_clone(struct nft_set_elem *elem);
 
 #define nft_set_elem_nlmsg_build_hdr	nft_nlmsg_build_hdr
 void nft_set_elems_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_set *s);
diff --git a/src/ruleset.c b/src/ruleset.c
index 89ea344..bc669c0 100644
--- a/src/ruleset.c
+++ b/src/ruleset.c
@@ -312,8 +312,14 @@ static int nft_ruleset_parse_set(struct nft_parse_ctx *ctx,
 				 struct nft_set *set, uint32_t type,
 				 struct nft_parse_err *err)
 {
+	struct nft_set *newset;
+
 	nft_set_attr_set_u32(set, NFT_SET_ATTR_ID, ctx->set_id++);
-	nft_set_list_add_tail(set, ctx->set_list);
+	newset = nft_set_clone(set);
+	if (newset == NULL)
+		goto err;
+
+	nft_set_list_add_tail(newset, ctx->set_list);
 
 	nft_ruleset_ctx_set_u32(ctx, NFT_RULESET_CTX_TYPE, type);
 	nft_ruleset_ctx_set(ctx, NFT_RULESET_CTX_SET, set);
diff --git a/src/set.c b/src/set.c
index c6c3301..5fd5245 100644
--- a/src/set.c
+++ b/src/set.c
@@ -249,6 +249,58 @@ uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr)
 }
 EXPORT_SYMBOL(nft_set_attr_get_u32);
 
+struct nft_set *nft_set_clone(const struct nft_set *set)
+{
+	struct nft_set *newset;
+	struct nft_set_elem *elem, *tmp, *newelem;
+
+	newset = nft_set_alloc();
+	if (newset == NULL)
+		return NULL;
+
+	if (set->flags & (1 << NFT_SET_ATTR_TABLE))
+		nft_set_attr_set_str(newset, NFT_SET_ATTR_TABLE, set->table);
+	if (set->flags & (1 << NFT_SET_ATTR_NAME))
+		nft_set_attr_set_str(newset, NFT_SET_ATTR_NAME, set->name);
+	if (set->flags & (1 << NFT_SET_ATTR_FLAGS))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_FLAGS,
+				     set->set_flags);
+	if (set->flags & (1 << NFT_SET_ATTR_KEY_TYPE))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_KEY_TYPE,
+				     set->key_type);
+	if (set->flags & (1 << NFT_SET_ATTR_KEY_LEN))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_KEY_LEN,
+				     set->key_len);
+	if (set->flags & (1 << NFT_SET_ATTR_DATA_TYPE))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DATA_TYPE,
+				     set->data_type);
+	if (set->flags & (1 << NFT_SET_ATTR_DATA_LEN))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DATA_LEN,
+				     set->data_len);
+	if (set->flags & (1 << NFT_SET_ATTR_FAMILY))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_FAMILY, set->family);
+	if (set->flags & (1 << NFT_SET_ATTR_ID))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_ID, set->id);
+	if (set->flags & (1 << NFT_SET_ATTR_POLICY))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_POLICY, set->policy);
+	if (set->flags & (1 << NFT_SET_ATTR_DESC_SIZE))
+		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DESC_SIZE,
+				     set->desc.size);
+
+	list_for_each_entry_safe(elem, tmp, &set->element_list, head) {
+		newelem = nft_set_elem_clone(elem);
+		if (newelem == NULL)
+			goto err;
+
+		list_add_tail(&newelem->head, &newset->element_list);
+	}
+
+	return newset;
+err:
+	nft_set_free(newset);
+	return NULL;
+}
+
 static void
 nft_set_nlmsg_build_desc_payload(struct nlmsghdr *nlh, struct nft_set *s)
 {
diff --git a/src/set_elem.c b/src/set_elem.c
index 5794f3a..1e3a157 100644
--- a/src/set_elem.c
+++ b/src/set_elem.c
@@ -164,6 +164,33 @@ uint32_t nft_set_elem_attr_get_u32(struct nft_set_elem *s, uint16_t attr)
 }
 EXPORT_SYMBOL(nft_set_elem_attr_get_u32);
 
+struct nft_set_elem *nft_set_elem_clone(struct nft_set_elem *elem)
+{
+	struct nft_set_elem *newelem;
+
+	newelem = nft_set_elem_alloc();
+	if (newelem == NULL)
+		return NULL;
+
+	if (elem->flags & (1 << NFT_SET_ELEM_ATTR_FLAGS))
+		nft_set_elem_attr_set_u32(newelem, NFT_SET_ELEM_ATTR_FLAGS,
+					  elem->set_elem_flags);
+	if (elem->flags & (1 << NFT_SET_ELEM_ATTR_KEY))
+		nft_set_elem_attr_set(newelem, NFT_SET_ELEM_ATTR_KEY,
+				      elem->key.val, elem->key.len);
+	if (elem->flags & (1 << NFT_SET_ELEM_ATTR_VERDICT))
+		nft_set_elem_attr_set_u32(newelem, NFT_SET_ELEM_ATTR_VERDICT,
+					  elem->data.verdict);
+	if (elem->flags & (1 << NFT_SET_ELEM_ATTR_CHAIN))
+		nft_set_elem_attr_set_str(newelem, NFT_SET_ELEM_ATTR_CHAIN,
+					  elem->data.chain);
+	if (elem->flags & (1 << NFT_SET_ELEM_ATTR_DATA))
+		nft_set_elem_attr_set(newelem, NFT_SET_ELEM_ATTR_DATA,
+				      elem->data.val, elem->data.len);
+
+	return newelem;
+}
+
 void nft_set_elem_nlmsg_build_payload(struct nlmsghdr *nlh,
 				      struct nft_set_elem *e)
 {
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list
  2015-02-17 14:03 [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list Alvaro Neira Ayuso
@ 2015-02-17 14:26 ` Pablo Neira Ayuso
  2015-02-17 14:28   ` Pablo Neira Ayuso
  0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2015-02-17 14:26 UTC (permalink / raw)
  To: Alvaro Neira Ayuso; +Cc: netfilter-devel

On Tue, Feb 17, 2015 at 03:03:51PM +0100, Alvaro Neira Ayuso wrote:
> When we parse a ruleset which has a rule using a set. First step is parse the
> set, set up an id and add it to a set list. Later, we use this set list to find
> the set associated to the rule and we set up the set id to the expression
> (lookup expression) of the rule.
> 
> The problem is if we return this set using the function
> nft_ruleset_parse_file_cb and we free this set. We have a crash when we try to
> iterate in the set list.
> 
> This patch solves it, cloning the set and adding the new set to the set list.
> 
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
> [changes in v2]
>  * Added the function to clone set and set elems.
> 
>  include/libnftnl/set.h |    2 ++
>  src/ruleset.c          |    8 +++++++-
>  src/set.c              |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/set_elem.c         |   27 +++++++++++++++++++++++++
>  4 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> index 7f3504f..3f8ef87 100644
> --- a/include/libnftnl/set.h
> +++ b/include/libnftnl/set.h
> @@ -42,6 +42,7 @@ const void *nft_set_attr_get_data(struct nft_set *s, uint16_t attr,
>  				  uint32_t *data_len);
>  const char *nft_set_attr_get_str(struct nft_set *s, uint16_t attr);
>  uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr);
> +struct nft_set *nft_set_clone(const struct nft_set *set);
>  
>  struct nlmsghdr;
>  
> @@ -103,6 +104,7 @@ const char *nft_set_elem_attr_get_str(struct nft_set_elem *s, uint16_t attr);
>  uint32_t nft_set_elem_attr_get_u32(struct nft_set_elem *s, uint16_t attr);
>  
>  bool nft_set_elem_attr_is_set(const struct nft_set_elem *s, uint16_t attr);
> +struct nft_set_elem *nft_set_elem_clone(struct nft_set_elem *elem);
>  
>  #define nft_set_elem_nlmsg_build_hdr	nft_nlmsg_build_hdr
>  void nft_set_elems_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_set *s);
> diff --git a/src/ruleset.c b/src/ruleset.c
> index 89ea344..bc669c0 100644
> --- a/src/ruleset.c
> +++ b/src/ruleset.c
> @@ -312,8 +312,14 @@ static int nft_ruleset_parse_set(struct nft_parse_ctx *ctx,
>  				 struct nft_set *set, uint32_t type,
>  				 struct nft_parse_err *err)
>  {
> +	struct nft_set *newset;
> +
>  	nft_set_attr_set_u32(set, NFT_SET_ATTR_ID, ctx->set_id++);
> -	nft_set_list_add_tail(set, ctx->set_list);
> +	newset = nft_set_clone(set);
> +	if (newset == NULL)
> +		goto err;
> +
> +	nft_set_list_add_tail(newset, ctx->set_list);
>  
>  	nft_ruleset_ctx_set_u32(ctx, NFT_RULESET_CTX_TYPE, type);
>  	nft_ruleset_ctx_set(ctx, NFT_RULESET_CTX_SET, set);
> diff --git a/src/set.c b/src/set.c
> index c6c3301..5fd5245 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -249,6 +249,58 @@ uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr)
>  }
>  EXPORT_SYMBOL(nft_set_attr_get_u32);
>  
> +struct nft_set *nft_set_clone(const struct nft_set *set)
> +{
> +	struct nft_set *newset;
> +	struct nft_set_elem *elem, *tmp, *newelem;
> +
> +	newset = nft_set_alloc();
> +	if (newset == NULL)
> +		return NULL;
> +

You better memcmpy() to save these many LOCs...

> +	if (set->flags & (1 << NFT_SET_ATTR_TABLE))
> +		nft_set_attr_set_str(newset, NFT_SET_ATTR_TABLE, set->table);
> +	if (set->flags & (1 << NFT_SET_ATTR_NAME))
> +		nft_set_attr_set_str(newset, NFT_SET_ATTR_NAME, set->name);
> +	if (set->flags & (1 << NFT_SET_ATTR_FLAGS))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_FLAGS,
> +				     set->set_flags);
> +	if (set->flags & (1 << NFT_SET_ATTR_KEY_TYPE))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_KEY_TYPE,
> +				     set->key_type);
> +	if (set->flags & (1 << NFT_SET_ATTR_KEY_LEN))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_KEY_LEN,
> +				     set->key_len);
> +	if (set->flags & (1 << NFT_SET_ATTR_DATA_TYPE))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DATA_TYPE,
> +				     set->data_type);
> +	if (set->flags & (1 << NFT_SET_ATTR_DATA_LEN))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DATA_LEN,
> +				     set->data_len);
> +	if (set->flags & (1 << NFT_SET_ATTR_FAMILY))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_FAMILY, set->family);
> +	if (set->flags & (1 << NFT_SET_ATTR_ID))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_ID, set->id);
> +	if (set->flags & (1 << NFT_SET_ATTR_POLICY))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_POLICY, set->policy);
> +	if (set->flags & (1 << NFT_SET_ATTR_DESC_SIZE))
> +		nft_set_attr_set_u32(newset, NFT_SET_ATTR_DESC_SIZE,
> +				     set->desc.size);

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list
  2015-02-17 14:26 ` Pablo Neira Ayuso
@ 2015-02-17 14:28   ` Pablo Neira Ayuso
  2015-02-17 14:35     ` Álvaro Neira Ayuso
  0 siblings, 1 reply; 4+ messages in thread
From: Pablo Neira Ayuso @ 2015-02-17 14:28 UTC (permalink / raw)
  To: Alvaro Neira Ayuso; +Cc: netfilter-devel

On Tue, Feb 17, 2015 at 03:26:09PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 17, 2015 at 03:03:51PM +0100, Alvaro Neira Ayuso wrote:
> > When we parse a ruleset which has a rule using a set. First step is parse the
> > set, set up an id and add it to a set list. Later, we use this set list to find
> > the set associated to the rule and we set up the set id to the expression
> > (lookup expression) of the rule.
> > 
> > The problem is if we return this set using the function
> > nft_ruleset_parse_file_cb and we free this set. We have a crash when we try to
> > iterate in the set list.
> > 
> > This patch solves it, cloning the set and adding the new set to the set list.
> > 
> > Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> > ---
> > [changes in v2]
> >  * Added the function to clone set and set elems.
> > 
> >  include/libnftnl/set.h |    2 ++
> >  src/ruleset.c          |    8 +++++++-
> >  src/set.c              |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/set_elem.c         |   27 +++++++++++++++++++++++++
> >  4 files changed, 88 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
> > index 7f3504f..3f8ef87 100644
> > --- a/include/libnftnl/set.h
> > +++ b/include/libnftnl/set.h
> > @@ -42,6 +42,7 @@ const void *nft_set_attr_get_data(struct nft_set *s, uint16_t attr,
> >  				  uint32_t *data_len);
> >  const char *nft_set_attr_get_str(struct nft_set *s, uint16_t attr);
> >  uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr);
> > +struct nft_set *nft_set_clone(const struct nft_set *set);
> >  
> >  struct nlmsghdr;
> >  
> > @@ -103,6 +104,7 @@ const char *nft_set_elem_attr_get_str(struct nft_set_elem *s, uint16_t attr);
> >  uint32_t nft_set_elem_attr_get_u32(struct nft_set_elem *s, uint16_t attr);
> >  
> >  bool nft_set_elem_attr_is_set(const struct nft_set_elem *s, uint16_t attr);
> > +struct nft_set_elem *nft_set_elem_clone(struct nft_set_elem *elem);
> >  
> >  #define nft_set_elem_nlmsg_build_hdr	nft_nlmsg_build_hdr
> >  void nft_set_elems_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_set *s);
> > diff --git a/src/ruleset.c b/src/ruleset.c
> > index 89ea344..bc669c0 100644
> > --- a/src/ruleset.c
> > +++ b/src/ruleset.c
> > @@ -312,8 +312,14 @@ static int nft_ruleset_parse_set(struct nft_parse_ctx *ctx,
> >  				 struct nft_set *set, uint32_t type,
> >  				 struct nft_parse_err *err)
> >  {
> > +	struct nft_set *newset;
> > +
> >  	nft_set_attr_set_u32(set, NFT_SET_ATTR_ID, ctx->set_id++);
> > -	nft_set_list_add_tail(set, ctx->set_list);
> > +	newset = nft_set_clone(set);
> > +	if (newset == NULL)
> > +		goto err;
> > +
> > +	nft_set_list_add_tail(newset, ctx->set_list);
> >  
> >  	nft_ruleset_ctx_set_u32(ctx, NFT_RULESET_CTX_TYPE, type);
> >  	nft_ruleset_ctx_set(ctx, NFT_RULESET_CTX_SET, set);
> > diff --git a/src/set.c b/src/set.c
> > index c6c3301..5fd5245 100644
> > --- a/src/set.c
> > +++ b/src/set.c
> > @@ -249,6 +249,58 @@ uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr)
> >  }
> >  EXPORT_SYMBOL(nft_set_attr_get_u32);
> >  
> > +struct nft_set *nft_set_clone(const struct nft_set *set)
> > +{
> > +	struct nft_set *newset;
> > +	struct nft_set_elem *elem, *tmp, *newelem;
> > +
> > +	newset = nft_set_alloc();
> > +	if (newset == NULL)
> > +		return NULL;
> > +
> 
> You better memcmpy() to save these many LOCs...
> 
> > +	if (set->flags & (1 << NFT_SET_ATTR_TABLE))
> > +		nft_set_attr_set_str(newset, NFT_SET_ATTR_TABLE, set->table);
> > +	if (set->flags & (1 << NFT_SET_ATTR_NAME))
> > +		nft_set_attr_set_str(newset, NFT_SET_ATTR_NAME, set->name);

Wait, you can memcpy of course. But make sure you clone fields that
are pointers too, otherwise both objects will pointer to the same
table and name. This will result in a crash if one of the objects is
releases and the second will have invalid references to released
memory.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list
  2015-02-17 14:28   ` Pablo Neira Ayuso
@ 2015-02-17 14:35     ` Álvaro Neira Ayuso
  0 siblings, 0 replies; 4+ messages in thread
From: Álvaro Neira Ayuso @ 2015-02-17 14:35 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netfilter-devel

El 17/02/15 a las 15:28, Pablo Neira Ayuso escribió:
> On Tue, Feb 17, 2015 at 03:26:09PM +0100, Pablo Neira Ayuso wrote:
>> On Tue, Feb 17, 2015 at 03:03:51PM +0100, Alvaro Neira Ayuso wrote:
>>> When we parse a ruleset which has a rule using a set. First step is parse the
>>> set, set up an id and add it to a set list. Later, we use this set list to find
>>> the set associated to the rule and we set up the set id to the expression
>>> (lookup expression) of the rule.
>>>
>>> The problem is if we return this set using the function
>>> nft_ruleset_parse_file_cb and we free this set. We have a crash when we try to
>>> iterate in the set list.
>>>
>>> This patch solves it, cloning the set and adding the new set to the set list.
>>>
>>> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>>> ---
>>> [changes in v2]
>>>   * Added the function to clone set and set elems.
>>>
>>>   include/libnftnl/set.h |    2 ++
>>>   src/ruleset.c          |    8 +++++++-
>>>   src/set.c              |   52 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>   src/set_elem.c         |   27 +++++++++++++++++++++++++
>>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/libnftnl/set.h b/include/libnftnl/set.h
>>> index 7f3504f..3f8ef87 100644
>>> --- a/include/libnftnl/set.h
>>> +++ b/include/libnftnl/set.h
>>> @@ -42,6 +42,7 @@ const void *nft_set_attr_get_data(struct nft_set *s, uint16_t attr,
>>>   				  uint32_t *data_len);
>>>   const char *nft_set_attr_get_str(struct nft_set *s, uint16_t attr);
>>>   uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr);
>>> +struct nft_set *nft_set_clone(const struct nft_set *set);
>>>
>>>   struct nlmsghdr;
>>>
>>> @@ -103,6 +104,7 @@ const char *nft_set_elem_attr_get_str(struct nft_set_elem *s, uint16_t attr);
>>>   uint32_t nft_set_elem_attr_get_u32(struct nft_set_elem *s, uint16_t attr);
>>>
>>>   bool nft_set_elem_attr_is_set(const struct nft_set_elem *s, uint16_t attr);
>>> +struct nft_set_elem *nft_set_elem_clone(struct nft_set_elem *elem);
>>>
>>>   #define nft_set_elem_nlmsg_build_hdr	nft_nlmsg_build_hdr
>>>   void nft_set_elems_nlmsg_build_payload(struct nlmsghdr *nlh, struct nft_set *s);
>>> diff --git a/src/ruleset.c b/src/ruleset.c
>>> index 89ea344..bc669c0 100644
>>> --- a/src/ruleset.c
>>> +++ b/src/ruleset.c
>>> @@ -312,8 +312,14 @@ static int nft_ruleset_parse_set(struct nft_parse_ctx *ctx,
>>>   				 struct nft_set *set, uint32_t type,
>>>   				 struct nft_parse_err *err)
>>>   {
>>> +	struct nft_set *newset;
>>> +
>>>   	nft_set_attr_set_u32(set, NFT_SET_ATTR_ID, ctx->set_id++);
>>> -	nft_set_list_add_tail(set, ctx->set_list);
>>> +	newset = nft_set_clone(set);
>>> +	if (newset == NULL)
>>> +		goto err;
>>> +
>>> +	nft_set_list_add_tail(newset, ctx->set_list);
>>>
>>>   	nft_ruleset_ctx_set_u32(ctx, NFT_RULESET_CTX_TYPE, type);
>>>   	nft_ruleset_ctx_set(ctx, NFT_RULESET_CTX_SET, set);
>>> diff --git a/src/set.c b/src/set.c
>>> index c6c3301..5fd5245 100644
>>> --- a/src/set.c
>>> +++ b/src/set.c
>>> @@ -249,6 +249,58 @@ uint32_t nft_set_attr_get_u32(struct nft_set *s, uint16_t attr)
>>>   }
>>>   EXPORT_SYMBOL(nft_set_attr_get_u32);
>>>
>>> +struct nft_set *nft_set_clone(const struct nft_set *set)
>>> +{
>>> +	struct nft_set *newset;
>>> +	struct nft_set_elem *elem, *tmp, *newelem;
>>> +
>>> +	newset = nft_set_alloc();
>>> +	if (newset == NULL)
>>> +		return NULL;
>>> +
>>
>> You better memcmpy() to save these many LOCs...
>>
>>> +	if (set->flags & (1 << NFT_SET_ATTR_TABLE))
>>> +		nft_set_attr_set_str(newset, NFT_SET_ATTR_TABLE, set->table);
>>> +	if (set->flags & (1 << NFT_SET_ATTR_NAME))
>>> +		nft_set_attr_set_str(newset, NFT_SET_ATTR_NAME, set->name);
>
> Wait, you can memcpy of course. But make sure you clone fields that
> are pointers too, otherwise both objects will pointer to the same
> table and name. This will result in a crash if one of the objects is
> releases and the second will have invalid references to released
> memory.

Ok, I'm going to take a look and rework the patch using memcpy.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2015-02-17 14:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-17 14:03 [libnftnl PATCH v2] ruleset: fix crash if we free sets included in the set_list Alvaro Neira Ayuso
2015-02-17 14:26 ` Pablo Neira Ayuso
2015-02-17 14:28   ` Pablo Neira Ayuso
2015-02-17 14:35     ` Álvaro Neira Ayuso

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).