subject:"\[PATCH nf\] netfilter\: ctnetlink\: make it safer when updating ct\->status"

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

Hi Feng,

2017-04-13 11:22 GMT+08:00 Gao Feng :
[...]
>> No, it's better to do this together, there are two invocations, it's not 
>> good to
>> copy these codes twice.
>
> You mean " on &= ~ IPS_UNCHANGEABLE_MASK " and " off &= ~ 
> IPS_UNCHANGEABLE_MASK " seems duplicated?

I see. I misunderstood your initial meaning. So I will send V2 :)
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

Hi Liping,

> -Original Message-
> From: Liping Zhang [mailto:zlpnob...@gmail.com]
> Sent: Thursday, April 13, 2017 11:15 AM
> To: Gao Feng <gfree.w...@foxmail.com>
> Cc: Liping Zhang <zlpnob...@163.com>; Pablo Neira Ayuso
> <pa...@netfilter.org>; Netfilter Developer Mailing List
> <netfilter-devel@vger.kernel.org>; cerne...@chromium.org
> Subject: Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> ct->status
> 
> Hi Feng,
> 
> 2017-04-13 10:42 GMT+08:00 Gao Feng <gfree.w...@foxmail.com>:
> [...]
> >> +static void
> >> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> >> +   unsigned long off) {
> >> + unsigned long mask;
> >> + unsigned int bit;
> >> +
> >> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
> >> + mask = 1 << bit;
> >> + /* Ignore these unchangable bits */
> >> + if (mask & IPS_UNCHANGEABLE_MASK)
> >> + continue;
> >
> > How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK
> > before loop.
> > Like "on &= ~ IPS_UNCHANGEABLE_MASK";
> > Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.
> 
> No, it's better to do this together, there are two invocations, it's not good 
> to
> copy these codes twice.

You mean " on &= ~ IPS_UNCHANGEABLE_MASK " and " off &= ~ IPS_UNCHANGEABLE_MASK 
" seems duplicated?

> 
> >
> > BTW, when some bits are set both of on and off, the "on" would be
> > effective, but "off" not.
> 
> This won't happen, see the invocation:
> 1. __ctnetlink_change_status(ct, status, 0); 2. __ctnetlink_change_status(ct,
> status, ~status);
> 
> > So I think we could use BUILD_BUG_ON to avoid it during building.
> > BUILD_BUG_ON(on);
> 
> Btw, this won't help, BUILD_BUG_ON is only effective on compile time, but
> "on" and "off" will be modified at the running time.
You are right.
This new function would be used frequently at running time.

Regards
Feng



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

Hi Feng,

2017-04-13 10:42 GMT+08:00 Gao Feng :
[...]
>> +static void
>> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
>> +   unsigned long off)
>> +{
>> + unsigned long mask;
>> + unsigned int bit;
>> +
>> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
>> + mask = 1 << bit;
>> + /* Ignore these unchangable bits */
>> + if (mask & IPS_UNCHANGEABLE_MASK)
>> + continue;
>
> How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK before
> loop.
> Like "on &= ~ IPS_UNCHANGEABLE_MASK";
> Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.

No, it's better to do this together, there are two invocations, it's not good to
copy these codes twice.

>
> BTW, when some bits are set both of on and off, the "on" would be effective,
> but "off" not.

This won't happen, see the invocation:
1. __ctnetlink_change_status(ct, status, 0);
2. __ctnetlink_change_status(ct, status, ~status);

> So I think we could use BUILD_BUG_ON to avoid it during building.
> BUILD_BUG_ON(on);

Btw, this won't help, BUILD_BUG_ON is only effective on compile time,
but "on" and "off" will be modified at the running time.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

> -Original Message-
> From: Gao Feng [mailto:gfree.w...@foxmail.com]
> Sent: Thursday, April 13, 2017 10:42 AM
> To: 'Liping Zhang' <zlpnob...@163.com>; 'pa...@netfilter.org'
> <pa...@netfilter.org>
> Cc: 'netfilter-devel@vger.kernel.org' <netfilter-devel@vger.kernel.org>;
> 'cerne...@chromium.org' <cerne...@chromium.org>; 'Liping Zhang'
> <zlpnob...@gmail.com>
> Subject: RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> ct->status
> 
> Hi Liping,
> 
> > -Original Message-
> > From: netfilter-devel-ow...@vger.kernel.org
> > [mailto:netfilter-devel-ow...@vger.kernel.org] On Behalf Of Liping
> > Zhang
> > Sent: Wednesday, April 12, 2017 11:57 PM
> > To: pa...@netfilter.org
> > Cc: netfilter-devel@vger.kernel.org; cerne...@chromium.org; Liping
> > Zhang <zlpnob...@gmail.com>
> > Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
> > ct->status
> >
> > From: Liping Zhang <zlpnob...@gmail.com>
> >
> > User can update the ct->status via nfnetlink, but using a non-atomic
> > operation "ct->status |= status;". This is unsafe, and may clear
> > IPS_DYING_BIT bit set by another CPU unexpectedly. For example:
> >  CPU0CPU1
> >   ctnetlink_change_status__nf_conntrack_find_get
> >   old = ct->status  nf_ct_gc_expired
> >   - nf_ct_kill
> >   -  test_and_set_bit(IPS_DYING_BIT
> >   - -
> >   ct->status = old | status;<-- oops, _DYING_ is cleared!
> >
> > So using a series of atomic bit operation to solve the above issue.
> >
> > Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.
> >
> > If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free,
> > but actually it is alloced by nf_conntrack_alloc.
> >
> > If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
> > deference, as the nfct_seqadj(ct) maybe NULL.
> >
> > So make these two bits be unchangable too.
> >
> > Last, add some comments to describe the logic change due to the commit
> > a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
> > processing"), which makes me feel a little confusing.
> >
> > Signed-off-by: Liping Zhang <zlpnob...@gmail.com>
> > ---
> >  include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
> >  net/netfilter/nf_conntrack_netlink.c   | 35
> > --
> >  2 files changed, 35 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > index 6a8e33d..38fc383 100644
> > --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> > +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> > @@ -82,10 +82,6 @@ enum ip_conntrack_status {
> > IPS_DYING_BIT = 9,
> > IPS_DYING = (1 << IPS_DYING_BIT),
> >
> > -   /* Bits that cannot be altered from userland. */
> > -   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> > -IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
> > -
> > /* Connection has fixed timeout. */
> > IPS_FIXED_TIMEOUT_BIT = 10,
> > IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT), @@ -101,6
> > +97,15 @@ enum ip_conntrack_status {
> > /* Conntrack got a helper explicitly attached via CT target. */
> > IPS_HELPER_BIT = 13,
> > IPS_HELPER = (1 << IPS_HELPER_BIT),
> > +
> > +   /* Be careful here, modifying these bits can make things messy,
> > +* so don't let users modify them directly.
> > +*/
> > +   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> > +IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
> > +IPS_SEQ_ADJUST | IPS_TEMPLATE),
> > +
> > +   __IPS_MAX_BIT = 14,
> >  };
> >
> >  /* Connection tracking event types */ diff --git
> > a/net/netfilter/nf_conntrack_netlink.c
> > b/net/netfilter/nf_conntrack_netlink.c
> > index 908d858..68efe1a 100644
> > --- a/net/netfilter/nf_conntrack_netlink.c
> > +++ b/net/netfilter/nf_conntrack_netlink.c
> > @@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
> > } #endif
> >
> > +static void
> > +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> >

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Gao Feng

Hi Liping,

> -Original Message-
> From: netfilter-devel-ow...@vger.kernel.org
> [mailto:netfilter-devel-ow...@vger.kernel.org] On Behalf Of Liping Zhang
> Sent: Wednesday, April 12, 2017 11:57 PM
> To: pa...@netfilter.org
> Cc: netfilter-devel@vger.kernel.org; cerne...@chromium.org; Liping Zhang
> <zlpnob...@gmail.com>
> Subject: [PATCH nf] netfilter: ctnetlink: make it safer when updating
ct->status
> 
> From: Liping Zhang <zlpnob...@gmail.com>
> 
> User can update the ct->status via nfnetlink, but using a non-atomic
operation
> "ct->status |= status;". This is unsafe, and may clear IPS_DYING_BIT bit
set by
> another CPU unexpectedly. For example:
>  CPU0CPU1
>   ctnetlink_change_status__nf_conntrack_find_get
>   old = ct->status  nf_ct_gc_expired
>   - nf_ct_kill
>   -  test_and_set_bit(IPS_DYING_BIT
>   - -
>   ct->status = old | status;<-- oops, _DYING_ is cleared!
> 
> So using a series of atomic bit operation to solve the above issue.
> 
> Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.
> 
> If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free, but
> actually it is alloced by nf_conntrack_alloc.
> 
> If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
deference,
> as the nfct_seqadj(ct) maybe NULL.
> 
> So make these two bits be unchangable too.
> 
> Last, add some comments to describe the logic change due to the commit
> a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
processing"),
> which makes me feel a little confusing.
> 
> Signed-off-by: Liping Zhang <zlpnob...@gmail.com>
> ---
>  include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
>  net/netfilter/nf_conntrack_netlink.c   | 35
> --
>  2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h
> b/include/uapi/linux/netfilter/nf_conntrack_common.h
> index 6a8e33d..38fc383 100644
> --- a/include/uapi/linux/netfilter/nf_conntrack_common.h
> +++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
> @@ -82,10 +82,6 @@ enum ip_conntrack_status {
>   IPS_DYING_BIT = 9,
>   IPS_DYING = (1 << IPS_DYING_BIT),
> 
> - /* Bits that cannot be altered from userland. */
> - IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> -  IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
> -
>   /* Connection has fixed timeout. */
>   IPS_FIXED_TIMEOUT_BIT = 10,
>   IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT), @@ -101,6
> +97,15 @@ enum ip_conntrack_status {
>   /* Conntrack got a helper explicitly attached via CT target. */
>   IPS_HELPER_BIT = 13,
>   IPS_HELPER = (1 << IPS_HELPER_BIT),
> +
> + /* Be careful here, modifying these bits can make things messy,
> +  * so don't let users modify them directly.
> +  */
> + IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
> +  IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
> +  IPS_SEQ_ADJUST | IPS_TEMPLATE),
> +
> + __IPS_MAX_BIT = 14,
>  };
> 
>  /* Connection tracking event types */
> diff --git a/net/netfilter/nf_conntrack_netlink.c
> b/net/netfilter/nf_conntrack_netlink.c
> index 908d858..68efe1a 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,  }
> #endif
> 
> +static void
> +__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
> +   unsigned long off)
> +{
> + unsigned long mask;
> + unsigned int bit;
> +
> + for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
> + mask = 1 << bit;
> + /* Ignore these unchangable bits */
> + if (mask & IPS_UNCHANGEABLE_MASK)
> + continue;

How about clear the bits of on and off with IPS_UNCHANGEABLE_MASK before
loop.
Like "on &= ~ IPS_UNCHANGEABLE_MASK";
Then the "if (mask & IPS_UNCHANGEABLE_MASK)" could be removed.

BTW, when some bits are set both of on and off, the "on" would be effective,
but "off" not.
So I think we could use BUILD_BUG_ON to avoid it during building.
BUILD_BUG_ON(on);

Best Regards
Feng

> +
> + if (on & mask)
> + set_bit(bit, >status);
> + else if (off & mask)
> +

[PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

2017-04-12 Thread Liping Zhang

From: Liping Zhang 

User can update the ct->status via nfnetlink, but using a non-atomic
operation "ct->status |= status;". This is unsafe, and may clear
IPS_DYING_BIT bit set by another CPU unexpectedly. For example:
 CPU0CPU1
  ctnetlink_change_status__nf_conntrack_find_get
  old = ct->status  nf_ct_gc_expired
  - nf_ct_kill
  -  test_and_set_bit(IPS_DYING_BIT
  - -
  ct->status = old | status;<-- oops, _DYING_ is cleared!

So using a series of atomic bit operation to solve the above issue.

Also note, user shouldn't set IPS_TEMPLATE, IPS_SEQ_ADJUST directly.

If we set the IPS_TEMPLATE_BIT, ct will be freed by nf_ct_tmpl_free,
but actually it is alloced by nf_conntrack_alloc.

If we set the IPS_SEQ_ADJUST_BIT, this may cause the NULL pointer
deference, as the nfct_seqadj(ct) maybe NULL.

So make these two bits be unchangable too.

Last, add some comments to describe the logic change due to the
commit a963d710f367 ("netfilter: ctnetlink: Fix regression in CTA_STATUS
processing"), which makes me feel a little confusing.

Signed-off-by: Liping Zhang 
---
 include/uapi/linux/netfilter/nf_conntrack_common.h | 13 +---
 net/netfilter/nf_conntrack_netlink.c   | 35 --
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h 
b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 6a8e33d..38fc383 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -82,10 +82,6 @@ enum ip_conntrack_status {
IPS_DYING_BIT = 9,
IPS_DYING = (1 << IPS_DYING_BIT),
 
-   /* Bits that cannot be altered from userland. */
-   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
-IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING),
-
/* Connection has fixed timeout. */
IPS_FIXED_TIMEOUT_BIT = 10,
IPS_FIXED_TIMEOUT = (1 << IPS_FIXED_TIMEOUT_BIT),
@@ -101,6 +97,15 @@ enum ip_conntrack_status {
/* Conntrack got a helper explicitly attached via CT target. */
IPS_HELPER_BIT = 13,
IPS_HELPER = (1 << IPS_HELPER_BIT),
+
+   /* Be careful here, modifying these bits can make things messy,
+* so don't let users modify them directly.
+*/
+   IPS_UNCHANGEABLE_MASK = (IPS_NAT_DONE_MASK | IPS_NAT_MASK |
+IPS_EXPECTED | IPS_CONFIRMED | IPS_DYING |
+IPS_SEQ_ADJUST | IPS_TEMPLATE),
+
+   __IPS_MAX_BIT = 14,
 };
 
 /* Connection tracking event types */
diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index 908d858..68efe1a 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1419,6 +1419,26 @@ ctnetlink_parse_nat_setup(struct nf_conn *ct,
 }
 #endif
 
+static void
+__ctnetlink_change_status(struct nf_conn *ct, unsigned long on,
+ unsigned long off)
+{
+   unsigned long mask;
+   unsigned int bit;
+
+   for (bit = 0; bit < __IPS_MAX_BIT; bit++) {
+   mask = 1 << bit;
+   /* Ignore these unchangable bits */
+   if (mask & IPS_UNCHANGEABLE_MASK)
+   continue;
+
+   if (on & mask)
+   set_bit(bit, >status);
+   else if (off & mask)
+   clear_bit(bit, >status);
+   }
+}
+
 static int
 ctnetlink_change_status(struct nf_conn *ct, const struct nlattr * const cda[])
 {
@@ -1438,10 +1458,7 @@ ctnetlink_change_status(struct nf_conn *ct, const struct 
nlattr * const cda[])
/* ASSURED bit can only be set */
return -EBUSY;
 
-   /* Be careful here, modifying NAT bits can screw up things,
-* so don't let users modify them directly if they don't pass
-* nf_nat_range. */
-   ct->status |= status & ~(IPS_NAT_DONE_MASK | IPS_NAT_MASK);
+   __ctnetlink_change_status(ct, status, 0);
return 0;
 }
 
@@ -1632,7 +1649,7 @@ ctnetlink_change_seq_adj(struct nf_conn *ct,
if (ret < 0)
return ret;
 
-   ct->status |= IPS_SEQ_ADJUST;
+   set_bit(IPS_SEQ_ADJUST_BIT, >status);
}
 
if (cda[CTA_SEQ_ADJ_REPLY]) {
@@ -1641,7 +1658,7 @@ ctnetlink_change_seq_adj(struct nf_conn *ct,
if (ret < 0)
return ret;
 
-   ct->status |= IPS_SEQ_ADJUST;
+   set_bit(IPS_SEQ_ADJUST_BIT, >status);
}
 
return 0;
@@ -2295,10 +2312,10 @@ ctnetlink_update_status(struct nf_conn *ct, const 
struct nlattr * const cda[])
/* This check is less strict than ctnetlink_change_status()
 *

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

Re: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

RE: [PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

[PATCH nf] netfilter: ctnetlink: make it safer when updating ct->status

6 matches

Site Navigation

Mail list logo

Footer information