Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
@ 2019-09-04 12:29 Fernando Fernandez Mancera
  2019-09-05 11:22 ` Pablo Neira Ayuso
  2019-11-01 14:42 ` Eric Garver
  0 siblings, 2 replies; 6+ messages in thread
From: Fernando Fernandez Mancera @ 2019-09-04 12:29 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Fernando Fernandez Mancera

Not all objects need an update operation. If the object type doesn't implement
an update operation and the user tries to update it there will be a EOPNOTSUPP
error instead of a null pointer.

Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful object update operation")
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 net/netfilter/nf_tables_api.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index cf767bc58e18..013d28899cab 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -5140,6 +5140,9 @@ static int nf_tables_updobj(const struct nft_ctx *ctx,
 	struct nft_trans *trans;
 	int err;
 
+	if (!obj->ops->update)
+		return -EOPNOTSUPP;
+
 	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
 				sizeof(struct nft_trans_obj));
 	if (!trans)
-- 
2.20.1


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

* Re: [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
  2019-09-04 12:29 [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update Fernando Fernandez Mancera
@ 2019-09-05 11:22 ` Pablo Neira Ayuso
  2019-11-01 14:42 ` Eric Garver
  1 sibling, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2019-09-05 11:22 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

On Wed, Sep 04, 2019 at 02:29:07PM +0200, Fernando Fernandez Mancera wrote:
> Not all objects need an update operation. If the object type doesn't implement
> an update operation and the user tries to update it there will be a EOPNOTSUPP
> error instead of a null pointer.

Applied, thanks.

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

* Re: [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
  2019-09-04 12:29 [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update Fernando Fernandez Mancera
  2019-09-05 11:22 ` Pablo Neira Ayuso
@ 2019-11-01 14:42 ` Eric Garver
  2019-11-01 15:01   ` Fernando Fernández Mancera
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Garver @ 2019-11-01 14:42 UTC (permalink / raw)
  To: Fernando Fernandez Mancera; +Cc: netfilter-devel

Hi Fernando,

On Wed, Sep 04, 2019 at 02:29:07PM +0200, Fernando Fernandez Mancera wrote:
> Not all objects need an update operation. If the object type doesn't implement
> an update operation and the user tries to update it there will be a EOPNOTSUPP
> error instead of a null pointer.
> 
> Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful object update operation")
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  net/netfilter/nf_tables_api.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index cf767bc58e18..013d28899cab 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -5140,6 +5140,9 @@ static int nf_tables_updobj(const struct nft_ctx *ctx,
>  	struct nft_trans *trans;
>  	int err;
>  
> +	if (!obj->ops->update)
> +		return -EOPNOTSUPP;
> +
>  	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
>  				sizeof(struct nft_trans_obj));
>  	if (!trans)
> -- 
> 2.20.1

I think this introduced a regression when adding an object that already
exists:

    # nft add table inet foobar
    # nft add counter inet foobar my_counter
    # nft add counter inet foobar my_counter
    Error: Could not process rule: Operation not supported
    add counter inet foobar my_counter
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It applies to all objects that don't provide an update handler; counter,
ct helper, ct timeout, ct exception, etc.


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

* Re: [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
  2019-11-01 14:42 ` Eric Garver
@ 2019-11-01 15:01   ` Fernando Fernández Mancera
  2019-11-01 15:11     ` Eric Garver
  0 siblings, 1 reply; 6+ messages in thread
From: Fernando Fernández Mancera @ 2019-11-01 15:01 UTC (permalink / raw)
  To: Eric Garver; +Cc: netfilter-devel

El 1 de noviembre de 2019 15:42:46 CET, Eric Garver <eric@garver.life> escribió:
>Hi Fernando,
>
>On Wed, Sep 04, 2019 at 02:29:07PM +0200, Fernando Fernandez Mancera
>wrote:
>> Not all objects need an update operation. If the object type doesn't
>implement
>> an update operation and the user tries to update it there will be a
>EOPNOTSUPP
>> error instead of a null pointer.
>> 
>> Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful object
>update operation")
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>>  net/netfilter/nf_tables_api.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/net/netfilter/nf_tables_api.c
>b/net/netfilter/nf_tables_api.c
>> index cf767bc58e18..013d28899cab 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -5140,6 +5140,9 @@ static int nf_tables_updobj(const struct
>nft_ctx *ctx,
>>  	struct nft_trans *trans;
>>  	int err;
>>  
>> +	if (!obj->ops->update)
>> +		return -EOPNOTSUPP;
>> +
>>  	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
>>  				sizeof(struct nft_trans_obj));
>>  	if (!trans)
>> -- 
>> 2.20.1
>
>I think this introduced a regression when adding an object that already
>exists:
>
>    # nft add table inet foobar
>    # nft add counter inet foobar my_counter
>    # nft add counter inet foobar my_counter
>    Error: Could not process rule: Operation not supported
>    add counter inet foobar my_counter
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
>It applies to all objects that don't provide an update handler;
>counter,
>ct helper, ct timeout, ct exception, etc.

Hi Eric,

It seems that you are right. What would be the behaviour here? Resets the object properties?

Thanks Eric!

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

* Re: [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
  2019-11-01 15:01   ` Fernando Fernández Mancera
@ 2019-11-01 15:11     ` Eric Garver
  2019-11-01 15:13       ` Fernando Fernández Mancera
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Garver @ 2019-11-01 15:11 UTC (permalink / raw)
  To: Fernando Fernández Mancera; +Cc: netfilter-devel

On Fri, Nov 01, 2019 at 04:01:51PM +0100, Fernando Fernández Mancera wrote:
> El 1 de noviembre de 2019 15:42:46 CET, Eric Garver <eric@garver.life> escribió:
> >Hi Fernando,
> >
> >On Wed, Sep 04, 2019 at 02:29:07PM +0200, Fernando Fernandez Mancera
> >wrote:
> >> Not all objects need an update operation. If the object type doesn't
> >implement
> >> an update operation and the user tries to update it there will be a
> >EOPNOTSUPP
> >> error instead of a null pointer.
> >> 
> >> Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful object
> >update operation")
> >> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> >> ---
> >>  net/netfilter/nf_tables_api.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/net/netfilter/nf_tables_api.c
> >b/net/netfilter/nf_tables_api.c
> >> index cf767bc58e18..013d28899cab 100644
> >> --- a/net/netfilter/nf_tables_api.c
> >> +++ b/net/netfilter/nf_tables_api.c
> >> @@ -5140,6 +5140,9 @@ static int nf_tables_updobj(const struct
> >nft_ctx *ctx,
> >>  	struct nft_trans *trans;
> >>  	int err;
> >>  
> >> +	if (!obj->ops->update)
> >> +		return -EOPNOTSUPP;
> >> +
> >>  	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
> >>  				sizeof(struct nft_trans_obj));
> >>  	if (!trans)
> >> -- 
> >> 2.20.1
> >
> >I think this introduced a regression when adding an object that already
> >exists:
> >
> >    # nft add table inet foobar
> >    # nft add counter inet foobar my_counter
> >    # nft add counter inet foobar my_counter
> >    Error: Could not process rule: Operation not supported
> >    add counter inet foobar my_counter
> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> >It applies to all objects that don't provide an update handler;
> >counter,
> >ct helper, ct timeout, ct exception, etc.
> 
> Hi Eric,
> 
> It seems that you are right. What would be the behaviour here? Resets the object properties?

I don't know what the correct behavior is in the kernel - maybe it
silently skips it. i.e. no attempt to update, but returns no error.

From a user perspective it should happily accept the re-add.

    # nft add table inet foobar
    # nft add counter inet foobar my_counter
    # nft add counter inet foobar my_counter
    ** no error **

Unless the "create" verb is used, then we should get an error:

    # nft create counter inet foobar my_counter
    Error: Could not process rule: File exists
    create counter inet foobar my_counter
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


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

* Re: [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update
  2019-11-01 15:11     ` Eric Garver
@ 2019-11-01 15:13       ` Fernando Fernández Mancera
  0 siblings, 0 replies; 6+ messages in thread
From: Fernando Fernández Mancera @ 2019-11-01 15:13 UTC (permalink / raw)
  To: Eric Garver; +Cc: netfilter-devel

El 1 de noviembre de 2019 16:11:59 CET, Eric Garver <eric@garver.life> escribió:
>On Fri, Nov 01, 2019 at 04:01:51PM +0100, Fernando Fernández Mancera
>wrote:
>> El 1 de noviembre de 2019 15:42:46 CET, Eric Garver
><eric@garver.life> escribió:
>> >Hi Fernando,
>> >
>> >On Wed, Sep 04, 2019 at 02:29:07PM +0200, Fernando Fernandez Mancera
>> >wrote:
>> >> Not all objects need an update operation. If the object type
>doesn't
>> >implement
>> >> an update operation and the user tries to update it there will be
>a
>> >EOPNOTSUPP
>> >> error instead of a null pointer.
>> >> 
>> >> Fixes: d62d0ba97b58 ("netfilter: nf_tables: Introduce stateful
>object
>> >update operation")
>> >> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> >> ---
>> >>  net/netfilter/nf_tables_api.c | 3 +++
>> >>  1 file changed, 3 insertions(+)
>> >> 
>> >> diff --git a/net/netfilter/nf_tables_api.c
>> >b/net/netfilter/nf_tables_api.c
>> >> index cf767bc58e18..013d28899cab 100644
>> >> --- a/net/netfilter/nf_tables_api.c
>> >> +++ b/net/netfilter/nf_tables_api.c
>> >> @@ -5140,6 +5140,9 @@ static int nf_tables_updobj(const struct
>> >nft_ctx *ctx,
>> >>  	struct nft_trans *trans;
>> >>  	int err;
>> >>  
>> >> +	if (!obj->ops->update)
>> >> +		return -EOPNOTSUPP;
>> >> +
>> >>  	trans = nft_trans_alloc(ctx, NFT_MSG_NEWOBJ,
>> >>  				sizeof(struct nft_trans_obj));
>> >>  	if (!trans)
>> >> -- 
>> >> 2.20.1
>> >
>> >I think this introduced a regression when adding an object that
>already
>> >exists:
>> >
>> >    # nft add table inet foobar
>> >    # nft add counter inet foobar my_counter
>> >    # nft add counter inet foobar my_counter
>> >    Error: Could not process rule: Operation not supported
>> >    add counter inet foobar my_counter
>> >    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >
>> >It applies to all objects that don't provide an update handler;
>> >counter,
>> >ct helper, ct timeout, ct exception, etc.
>> 
>> Hi Eric,
>> 
>> It seems that you are right. What would be the behaviour here? Resets
>the object properties?
>
>I don't know what the correct behavior is in the kernel - maybe it
>silently skips it. i.e. no attempt to update, but returns no error.
>
>From a user perspective it should happily accept the re-add.
>
>    # nft add table inet foobar
>    # nft add counter inet foobar my_counter
>    # nft add counter inet foobar my_counter
>    ** no error **
>
>Unless the "create" verb is used, then we should get an error:
>
>    # nft create counter inet foobar my_counter
>    Error: Could not process rule: File exists
>    create counter inet foobar my_counter
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Sure, I am going to prepare a patch for this. Sorry about the regression. Thanks!

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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 12:29 [PATCH nf-next v2] netfilter: nf_tables: fix possible null-pointer dereference in object update Fernando Fernandez Mancera
2019-09-05 11:22 ` Pablo Neira Ayuso
2019-11-01 14:42 ` Eric Garver
2019-11-01 15:01   ` Fernando Fernández Mancera
2019-11-01 15:11     ` Eric Garver
2019-11-01 15:13       ` Fernando Fernández Mancera

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git