Re: [PATCH V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Paul Moore 
On Thu, Feb 23, 2017 at 8:59 PM, Florian Westphal  wrote:
> Richard Guy Briggs  wrote:
>> > Not following, sorry, are you saying users can/should use -j MARK
>> > somehow?
>>
>> Part of the discussed design and rationale for stripping many of the
>> vanishing fields is that when setting up netfilter rules to invoke the
>> AUDIT target, an accompanying nf mark should be used to indicate which
>> rule caught that packet, since the chain name and rule number aren't
>> available to the audit target.  We would use the nf mark similarly to
>> the way we use a rule key in the audit rules (see man auditctl).
>
> I see.  While this works, nfmark might already be used for other
> purposes such as policy routing, so you might need an extra cookie
> that can be passed to the AUDIT target instead.

Yes, we discussed the idea that the nfmark field already serves many
purposes, most of which are related to labeling traffic flows.  I
agree that using the nfmark may complicate some configurations, but
using it in this manner seems to be in keeping with the ideas behind
nfmark (from what I can tell).  As for the configuration complexity, I
think it is safe to say that any users of the NETFILTER_PKT record
already have a sufficiently complex system configuration and the added
complexity here may not be significant; in fact, the existing nfmark
configuration may be helpful in identifying traffic categories such
that no changes need to be made.

-- 
paul moore
www.paul-moore.com
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Florian Westphal 
Richard Guy Briggs  wrote:
> > Not following, sorry, are you saying users can/should use -j MARK
> > somehow?
> 
> Part of the discussed design and rationale for stripping many of the
> vanishing fields is that when setting up netfilter rules to invoke the
> AUDIT target, an accompanying nf mark should be used to indicate which
> rule caught that packet, since the chain name and rule number aren't
> available to the audit target.  We would use the nf mark similarly to
> the way we use a rule key in the audit rules (see man auditctl).

I see.  While this works, nfmark might already be used for other
purposes such as policy routing, so you might need an extra cookie
that can be passed to the AUDIT target instead.
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Florian Westphal 
Paul Moore  wrote:
> On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs  wrote:
> > I had another idea on how to include the sport and dport and that was to
> > use the same identifier for sport/icmptype and also for dport/icmpcode,
> > but you've already said you are not interested.
> 
> Not at this point in time since we don't have any good requirements at
> the moment.  I would like us to keep this small until we have a better
> idea of how people want to use this, this way we don't end up stuck
> maintaining something that is ill suited for what people actually
> want/use.

Right, I think people that want more info should just use NFLOG to
dump the packet to userspace, extracting all the stuff in kernel is
just a mess.
--
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

[PATCH nf 1/2] netfilter: nf_ct_expect: nf_ct_expect_related_report(): Return zero on success.

2017-02-23 Thread Jarno Rajahalme 
Commit 4dee62b1b9b4 ("netfilter: nf_ct_expect: nf_ct_expect_insert()
returns void") inadvertently changed the successful return value of
nf_ct_expect_related_report() from 0 to 1, which caused openvswitch
conntrack integration fail in FTP test cases.

Fix this by always returning zero on the success code path.

Fixes: 4dee62b1b9b4 ("netfilter: nf_ct_expect: nf_ct_expect_insert() returns 
void")
Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
---
 net/netfilter/nf_conntrack_expect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_conntrack_expect.c 
b/net/netfilter/nf_conntrack_expect.c
index e19a697..d6ace69 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -467,7 +467,7 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect 
*expect,
 
spin_unlock_bh(_conntrack_expect_lock);
nf_ct_expect_event_report(IPEXP_NEW, expect, portid, report);
-   return ret;
+   return 0;
 out:
spin_unlock_bh(_conntrack_expect_lock);
return ret;
-- 
2.1.4

--
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

[PATCH nf 2/2] netfilter: nf_ct_expect: Change __nf_ct_expect_check() return value.

2017-02-23 Thread Jarno Rajahalme 
Commit 4dee62b1b9b4 ("netfilter: nf_ct_expect: nf_ct_expect_insert()
returns void") inadvertently changed the successful return value of
nf_ct_expect_related_report() from 0 to 1 due to
__nf_ct_expect_check() returning 1 on success.  Prevent this
regression in the future by changing the return value of
__nf_ct_expect_check() to 0 on success.

Signed-off-by: Jarno Rajahalme 
Acked-by: Joe Stringer 
---
 net/netfilter/nf_conntrack_expect.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_expect.c 
b/net/netfilter/nf_conntrack_expect.c
index d6ace69..4b2e1fb 100644
--- a/net/netfilter/nf_conntrack_expect.c
+++ b/net/netfilter/nf_conntrack_expect.c
@@ -410,7 +410,7 @@ static inline int __nf_ct_expect_check(struct 
nf_conntrack_expect *expect)
struct net *net = nf_ct_exp_net(expect);
struct hlist_node *next;
unsigned int h;
-   int ret = 1;
+   int ret = 0;
 
if (!master_help) {
ret = -ESHUTDOWN;
@@ -460,7 +460,7 @@ int nf_ct_expect_related_report(struct nf_conntrack_expect 
*expect,
 
spin_lock_bh(_conntrack_expect_lock);
ret = __nf_ct_expect_check(expect);
-   if (ret <= 0)
+   if (ret < 0)
goto out;
 
nf_ct_expect_insert(expect);
-- 
2.1.4

--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Paul Moore 
On Thu, Feb 23, 2017 at 12:35 PM, Richard Guy Briggs  wrote:
> On 2017-02-23 12:14, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs  wrote:
>> > On 2017-02-23 12:06, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs  
>> >> wrote:
>> >> > On 2017-02-23 11:57, Paul Moore wrote:
>> >> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  
>> >> >> wrote:
>> >> >> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> >> >> Richard Guy Briggs  wrote:
>> >> >> >> > Simplify and eliminate flipping in and out of message fields, 
>> >> >> >> > relying on nfmark
>> >> >> >> > the way we do for audit_key.
>> >> >> >> >
>> >> >> >> > +struct nfpkt_par {
>> >> >> >> > +   int ipv;
>> >> >> >> > +   const void *saddr;
>> >> >> >> > +   const void *daddr;
>> >> >> >> > +   u8 proto;
>> >> >> >> > +};
>> >> >> >>
>> >> >> >> This is problematic, see below for why.
>> >> >> >>
>> >> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff 
>> >> >> >> > *skb)
>> >> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff 
>> >> >> >> > *skb, struct nfpkt_par *apar)
>> >> >> >> >  {
>> >> >> >> > struct iphdr _iph;
>> >> >> >> > const struct iphdr *ih;
>> >> >> >> >
>> >> >> >> > +   apar->ipv = 4;
>> >> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> >> >> > -   if (!ih) {
>> >> >> >> > -   audit_log_format(ab, " truncated=1");
>> >> >> >> > +   if (!ih)
>> >> >> >> > return;
>> >> >> >>
>> >> >> >> Removing this "truncated" has the consequence that this can later 
>> >> >> >> log
>> >> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >> >> >>
>> >> >> >> This cannot happen for ip(6)tables because ip stack discards broken 
>> >> >> >> l3 headers
>> >> >> >> before the netfilter hooks get called, but its possible with 
>> >> >> >> NFPROTO_BRIDGE.
>> >> >> >>
>> >> >> >> Perhaps you will need to change audit_ip4/6 to return "false" when 
>> >> >> >> it can't
>> >> >> >> get the l3 information now so we only log zero addresses when the 
>> >> >> >> packet
>> >> >> >> really did contain them.
>> >> >> >
>> >> >> > Ok, to clarify the implications, are you saying that handing a NULL
>> >> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or 
>> >> >> > "?"
>> >> >>
>> >> >> My initial reaction is that if the packet is so badly
>> >> >> truncated/malformed that we don't have a full IP header than we should
>> >> >> just refrain from logging the packet; it's too malformed/garbage to
>> >> >> offer any useful information and the normal packet processing should
>> >> >> result in the packet being discarded anyway.
>> >> >
>> >> > Which is why I wanted the ethertype, but that can be coded into the 
>> >> > nfmark.
>> >>
>> >> If the packet is garbage (garbage without any payload in this case),
>> >> what does it matter?  It's noise.
>> >
>> > It could be an indicator that either the logging rules or the filter
>> > rules need honing, or even that there is a bug in the network code.
>>
>> Elaborate on this please, I still don't see how logging the ethertype
>> is helpful for a malformed packet.
>
> Well, since we can encode it in the nfmark, it could be helpful, but not 
> necessary.
>
> Each bit of information we can include in the audit log message removes
> something we need to code in the nf mark.  That's why things like ifin,
> ifout, action, hook are easy to include and help reduce the amount of
> nf mark coding needed when devising netfilter rules.

... but if the packet is so badly manged it doesn't even have a
complete IP header, what's the point in logging the packet at all?
That's the point I'm trying to make.

> I had another idea on how to include the sport and dport and that was to
> use the same identifier for sport/icmptype and also for dport/icmpcode,
> but you've already said you are not interested.

Not at this point in time since we don't have any good requirements at
the moment.  I would like us to keep this small until we have a better
idea of how people want to use this, this way we don't end up stuck
maintaining something that is ill suited for what people actually
want/use.

-- 
paul moore
www.paul-moore.com
--
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

[PATCH] uapi: fix linux/netfilter/xt_hashlimit.h userspace compilation error

2017-02-23 Thread Dmitry V. Levin 
Include  like some of uapi/linux/netfilter/xt_*.h
headers do to fix the following linux/netfilter/xt_hashlimit.h
userspace compilation error:

/usr/include/linux/netfilter/xt_hashlimit.h:90:12: error: 'NAME_MAX' undeclared 
here (not in a function)
  char name[NAME_MAX];

Signed-off-by: Dmitry V. Levin 
---
 include/uapi/linux/netfilter/xt_hashlimit.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/netfilter/xt_hashlimit.h 
b/include/uapi/linux/netfilter/xt_hashlimit.h
index 3efc0ca..79da349 100644
--- a/include/uapi/linux/netfilter/xt_hashlimit.h
+++ b/include/uapi/linux/netfilter/xt_hashlimit.h
@@ -2,6 +2,7 @@
 #define _UAPI_XT_HASHLIMIT_H
 
 #include 
+#include 
 #include 
 
 /* timings are in milliseconds. */
-- 
ldv
--
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

[PATCH libnftnl] exthdr: remove unused variable uval8

2017-02-23 Thread Alexander Alemayhu 
Was added but not used in d7b451fe1a45 (src: add TCP option matching
requirements, 2017-02-07). Fixes the following warning:

expr/exthdr.c: In function ‘nftnl_expr_exthdr_json_parse’:
expr/exthdr.c:244:10: warning: unused variable ‘uval8’ [-Wunused-variable]
  uint8_t uval8;
  ^

Signed-off-by: Alexander Alemayhu 
---
 src/expr/exthdr.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/expr/exthdr.c b/src/expr/exthdr.c
index c44c1a75a5ea..9ed4ae1725ac 100644
--- a/src/expr/exthdr.c
+++ b/src/expr/exthdr.c
@@ -241,7 +241,6 @@ nftnl_expr_exthdr_json_parse(struct nftnl_expr *e, json_t 
*root,
 #ifdef JSON_PARSING
const char *exthdr_type;
uint32_t uval32;
-   uint8_t uval8;
int type;
 
if (nftnl_jansson_parse_reg(root, "dreg", NFTNL_TYPE_U32, ,
-- 
2.9.3

--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 12:20, Steve Grubb wrote:
> On Wednesday, February 22, 2017 9:50:54 PM EST Richard Guy Briggs wrote:
> > Simplify and eliminate flipping in and out of message fields, relying on
> > nfmark the way we do for audit_key.
> > 
> > https://github.com/linux-audit/audit-kernel/issues/11
> > 
> > Signed-off-by: Richard Guy Briggs 
> 
> If this is reworked, do you mind including a raw log event in the explanation 
> part of the patch? I need to see the resulting event to see how user space 
> needs to adapt.

Yes, I'll make a note to remember to add that.

> -Steve

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Steve Grubb 
On Wednesday, February 22, 2017 9:50:54 PM EST Richard Guy Briggs wrote:
> Simplify and eliminate flipping in and out of message fields, relying on
> nfmark the way we do for audit_key.
> 
> https://github.com/linux-audit/audit-kernel/issues/11
> 
> Signed-off-by: Richard Guy Briggs 

If this is reworked, do you mind including a raw log event in the explanation 
part of the patch? I need to see the resulting event to see how user space 
needs to adapt.

Thanks,
-Steve

--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 18:06, Florian Westphal wrote:
> Richard Guy Briggs  wrote:
> > On 2017-02-23 11:57, Paul Moore wrote:
> > > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  
> > > wrote:
> > > > On 2017-02-23 06:20, Florian Westphal wrote:
> > > >> Richard Guy Briggs  wrote:
> > > >> > Simplify and eliminate flipping in and out of message fields, 
> > > >> > relying on nfmark
> > > >> > the way we do for audit_key.
> > > >> >
> > > >> > +struct nfpkt_par {
> > > >> > +   int ipv;
> > > >> > +   const void *saddr;
> > > >> > +   const void *daddr;
> > > >> > +   u8 proto;
> > > >> > +};
> > > >>
> > > >> This is problematic, see below for why.
> > > >>
> > > >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > > >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
> > > >> > struct nfpkt_par *apar)
> > > >> >  {
> > > >> > struct iphdr _iph;
> > > >> > const struct iphdr *ih;
> > > >> >
> > > >> > +   apar->ipv = 4;
> > > >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > > >> > -   if (!ih) {
> > > >> > -   audit_log_format(ab, " truncated=1");
> > > >> > +   if (!ih)
> > > >> > return;
> > > >>
> > > >> Removing this "truncated" has the consequence that this can later log
> > > >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> > > >>
> > > >> This cannot happen for ip(6)tables because ip stack discards broken l3 
> > > >> headers
> > > >> before the netfilter hooks get called, but its possible with 
> > > >> NFPROTO_BRIDGE.
> > > >>
> > > >> Perhaps you will need to change audit_ip4/6 to return "false" when it 
> > > >> can't
> > > >> get the l3 information now so we only log zero addresses when the 
> > > >> packet
> > > >> really did contain them.
> > > >
> > > > Ok, to clarify the implications, are you saying that handing a NULL
> > > > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
> 
> No, if you pass pointers that would indeed log NULL.
> 
> > > My initial reaction is that if the packet is so badly
> > > truncated/malformed that we don't have a full IP header than we should
> > > just refrain from logging the packet; it's too malformed/garbage to
> > > offer any useful information and the normal packet processing should
> > > result in the packet being discarded anyway.
> 
> True for ip/ipv6, not sure about bridge though.
> 
> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
> 
> Not following, sorry, are you saying users can/should use -j MARK
> somehow?

Part of the discussed design and rationale for stripping many of the
vanishing fields is that when setting up netfilter rules to invoke the
AUDIT target, an accompanying nf mark should be used to indicate which
rule caught that packet, since the chain name and rule number aren't
available to the audit target.  We would use the nf mark similarly to
the way we use a rule key in the audit rules (see man auditctl).

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Paul Moore 
On Thu, Feb 23, 2017 at 12:13 PM, Richard Guy Briggs  wrote:
> On 2017-02-23 12:06, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs  wrote:
>> > On 2017-02-23 11:57, Paul Moore wrote:
>> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  
>> >> wrote:
>> >> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> >> Richard Guy Briggs  wrote:
>> >> >> > Simplify and eliminate flipping in and out of message fields, 
>> >> >> > relying on nfmark
>> >> >> > the way we do for audit_key.
>> >> >> >
>> >> >> > +struct nfpkt_par {
>> >> >> > +   int ipv;
>> >> >> > +   const void *saddr;
>> >> >> > +   const void *daddr;
>> >> >> > +   u8 proto;
>> >> >> > +};
>> >> >>
>> >> >> This is problematic, see below for why.
>> >> >>
>> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
>> >> >> > struct nfpkt_par *apar)
>> >> >> >  {
>> >> >> > struct iphdr _iph;
>> >> >> > const struct iphdr *ih;
>> >> >> >
>> >> >> > +   apar->ipv = 4;
>> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> >> > -   if (!ih) {
>> >> >> > -   audit_log_format(ab, " truncated=1");
>> >> >> > +   if (!ih)
>> >> >> > return;
>> >> >>
>> >> >> Removing this "truncated" has the consequence that this can later log
>> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >> >>
>> >> >> This cannot happen for ip(6)tables because ip stack discards broken l3 
>> >> >> headers
>> >> >> before the netfilter hooks get called, but its possible with 
>> >> >> NFPROTO_BRIDGE.
>> >> >>
>> >> >> Perhaps you will need to change audit_ip4/6 to return "false" when it 
>> >> >> can't
>> >> >> get the l3 information now so we only log zero addresses when the 
>> >> >> packet
>> >> >> really did contain them.
>> >> >
>> >> > Ok, to clarify the implications, are you saying that handing a NULL
>> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
>> >>
>> >> My initial reaction is that if the packet is so badly
>> >> truncated/malformed that we don't have a full IP header than we should
>> >> just refrain from logging the packet; it's too malformed/garbage to
>> >> offer any useful information and the normal packet processing should
>> >> result in the packet being discarded anyway.
>> >
>> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
>>
>> If the packet is garbage (garbage without any payload in this case),
>> what does it matter?  It's noise.
>
> It could be an indicator that either the logging rules or the filter
> rules need honing, or even that there is a bug in the network code.

Elaborate on this please, I still don't see how logging the ethertype
is helpful for a malformed packet.

-- 
paul moore
www.paul-moore.com
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 12:06, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs  wrote:
> > On 2017-02-23 11:57, Paul Moore wrote:
> >> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  
> >> wrote:
> >> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> >> Richard Guy Briggs  wrote:
> >> >> > Simplify and eliminate flipping in and out of message fields, relying 
> >> >> > on nfmark
> >> >> > the way we do for audit_key.
> >> >> >
> >> >> > +struct nfpkt_par {
> >> >> > +   int ipv;
> >> >> > +   const void *saddr;
> >> >> > +   const void *daddr;
> >> >> > +   u8 proto;
> >> >> > +};
> >> >>
> >> >> This is problematic, see below for why.
> >> >>
> >> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
> >> >> > struct nfpkt_par *apar)
> >> >> >  {
> >> >> > struct iphdr _iph;
> >> >> > const struct iphdr *ih;
> >> >> >
> >> >> > +   apar->ipv = 4;
> >> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> >> > -   if (!ih) {
> >> >> > -   audit_log_format(ab, " truncated=1");
> >> >> > +   if (!ih)
> >> >> > return;
> >> >>
> >> >> Removing this "truncated" has the consequence that this can later log
> >> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> >> >>
> >> >> This cannot happen for ip(6)tables because ip stack discards broken l3 
> >> >> headers
> >> >> before the netfilter hooks get called, but its possible with 
> >> >> NFPROTO_BRIDGE.
> >> >>
> >> >> Perhaps you will need to change audit_ip4/6 to return "false" when it 
> >> >> can't
> >> >> get the l3 information now so we only log zero addresses when the packet
> >> >> really did contain them.
> >> >
> >> > Ok, to clarify the implications, are you saying that handing a NULL
> >> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
> >>
> >> My initial reaction is that if the packet is so badly
> >> truncated/malformed that we don't have a full IP header than we should
> >> just refrain from logging the packet; it's too malformed/garbage to
> >> offer any useful information and the normal packet processing should
> >> result in the packet being discarded anyway.
> >
> > Which is why I wanted the ethertype, but that can be coded into the nfmark.
> 
> If the packet is garbage (garbage without any payload in this case),
> what does it matter?  It's noise.

It could be an indicator that either the logging rules or the filter
rules need honing, or even that there is a bug in the network code.

> paul moore

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Paul Moore 
On Thu, Feb 23, 2017 at 12:04 PM, Richard Guy Briggs  wrote:
> On 2017-02-23 11:57, Paul Moore wrote:
>> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  wrote:
>> > On 2017-02-23 06:20, Florian Westphal wrote:
>> >> Richard Guy Briggs  wrote:
>> >> > Simplify and eliminate flipping in and out of message fields, relying 
>> >> > on nfmark
>> >> > the way we do for audit_key.
>> >> >
>> >> > +struct nfpkt_par {
>> >> > +   int ipv;
>> >> > +   const void *saddr;
>> >> > +   const void *daddr;
>> >> > +   u8 proto;
>> >> > +};
>> >>
>> >> This is problematic, see below for why.
>> >>
>> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
>> >> > struct nfpkt_par *apar)
>> >> >  {
>> >> > struct iphdr _iph;
>> >> > const struct iphdr *ih;
>> >> >
>> >> > +   apar->ipv = 4;
>> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> >> > -   if (!ih) {
>> >> > -   audit_log_format(ab, " truncated=1");
>> >> > +   if (!ih)
>> >> > return;
>> >>
>> >> Removing this "truncated" has the consequence that this can later log
>> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>> >>
>> >> This cannot happen for ip(6)tables because ip stack discards broken l3 
>> >> headers
>> >> before the netfilter hooks get called, but its possible with 
>> >> NFPROTO_BRIDGE.
>> >>
>> >> Perhaps you will need to change audit_ip4/6 to return "false" when it 
>> >> can't
>> >> get the l3 information now so we only log zero addresses when the packet
>> >> really did contain them.
>> >
>> > Ok, to clarify the implications, are you saying that handing a NULL
>> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
>>
>> My initial reaction is that if the packet is so badly
>> truncated/malformed that we don't have a full IP header than we should
>> just refrain from logging the packet; it's too malformed/garbage to
>> offer any useful information and the normal packet processing should
>> result in the packet being discarded anyway.
>
> Which is why I wanted the ethertype, but that can be coded into the nfmark.

If the packet is garbage (garbage without any payload in this case),
what does it matter?  It's noise.

-- 
paul moore
www.paul-moore.com
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Florian Westphal 
Richard Guy Briggs  wrote:
> On 2017-02-23 11:57, Paul Moore wrote:
> > On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  
> > wrote:
> > > On 2017-02-23 06:20, Florian Westphal wrote:
> > >> Richard Guy Briggs  wrote:
> > >> > Simplify and eliminate flipping in and out of message fields, relying 
> > >> > on nfmark
> > >> > the way we do for audit_key.
> > >> >
> > >> > +struct nfpkt_par {
> > >> > +   int ipv;
> > >> > +   const void *saddr;
> > >> > +   const void *daddr;
> > >> > +   u8 proto;
> > >> > +};
> > >>
> > >> This is problematic, see below for why.
> > >>
> > >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
> > >> > struct nfpkt_par *apar)
> > >> >  {
> > >> > struct iphdr _iph;
> > >> > const struct iphdr *ih;
> > >> >
> > >> > +   apar->ipv = 4;
> > >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > >> > -   if (!ih) {
> > >> > -   audit_log_format(ab, " truncated=1");
> > >> > +   if (!ih)
> > >> > return;
> > >>
> > >> Removing this "truncated" has the consequence that this can later log
> > >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> > >>
> > >> This cannot happen for ip(6)tables because ip stack discards broken l3 
> > >> headers
> > >> before the netfilter hooks get called, but its possible with 
> > >> NFPROTO_BRIDGE.
> > >>
> > >> Perhaps you will need to change audit_ip4/6 to return "false" when it 
> > >> can't
> > >> get the l3 information now so we only log zero addresses when the packet
> > >> really did contain them.
> > >
> > > Ok, to clarify the implications, are you saying that handing a NULL
> > > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"

No, if you pass pointers that would indeed log NULL.

> > My initial reaction is that if the packet is so badly
> > truncated/malformed that we don't have a full IP header than we should
> > just refrain from logging the packet; it's too malformed/garbage to
> > offer any useful information and the normal packet processing should
> > result in the packet being discarded anyway.

True for ip/ipv6, not sure about bridge though.

> Which is why I wanted the ethertype, but that can be coded into the nfmark.

Not following, sorry, are you saying users can/should use -j MARK
somehow?
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 11:57, Paul Moore wrote:
> On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  wrote:
> > On 2017-02-23 06:20, Florian Westphal wrote:
> >> Richard Guy Briggs  wrote:
> >> > Simplify and eliminate flipping in and out of message fields, relying on 
> >> > nfmark
> >> > the way we do for audit_key.
> >> >
> >> > +struct nfpkt_par {
> >> > +   int ipv;
> >> > +   const void *saddr;
> >> > +   const void *daddr;
> >> > +   u8 proto;
> >> > +};
> >>
> >> This is problematic, see below for why.
> >>
> >> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> >> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
> >> > struct nfpkt_par *apar)
> >> >  {
> >> > struct iphdr _iph;
> >> > const struct iphdr *ih;
> >> >
> >> > +   apar->ipv = 4;
> >> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> >> > -   if (!ih) {
> >> > -   audit_log_format(ab, " truncated=1");
> >> > +   if (!ih)
> >> > return;
> >>
> >> Removing this "truncated" has the consequence that this can later log
> >> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> >>
> >> This cannot happen for ip(6)tables because ip stack discards broken l3 
> >> headers
> >> before the netfilter hooks get called, but its possible with 
> >> NFPROTO_BRIDGE.
> >>
> >> Perhaps you will need to change audit_ip4/6 to return "false" when it can't
> >> get the l3 information now so we only log zero addresses when the packet
> >> really did contain them.
> >
> > Ok, to clarify the implications, are you saying that handing a NULL
> > pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"
> 
> My initial reaction is that if the packet is so badly
> truncated/malformed that we don't have a full IP header than we should
> just refrain from logging the packet; it's too malformed/garbage to
> offer any useful information and the normal packet processing should
> result in the packet being discarded anyway.

Which is why I wanted the ethertype, but that can be coded into the nfmark.

> paul moore

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Paul Moore 
On Thu, Feb 23, 2017 at 10:51 AM, Richard Guy Briggs  wrote:
> On 2017-02-23 06:20, Florian Westphal wrote:
>> Richard Guy Briggs  wrote:
>> > Simplify and eliminate flipping in and out of message fields, relying on 
>> > nfmark
>> > the way we do for audit_key.
>> >
>> > +struct nfpkt_par {
>> > +   int ipv;
>> > +   const void *saddr;
>> > +   const void *daddr;
>> > +   u8 proto;
>> > +};
>>
>> This is problematic, see below for why.
>>
>> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
>> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, 
>> > struct nfpkt_par *apar)
>> >  {
>> > struct iphdr _iph;
>> > const struct iphdr *ih;
>> >
>> > +   apar->ipv = 4;
>> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
>> > -   if (!ih) {
>> > -   audit_log_format(ab, " truncated=1");
>> > +   if (!ih)
>> > return;
>>
>> Removing this "truncated" has the consequence that this can later log
>> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
>>
>> This cannot happen for ip(6)tables because ip stack discards broken l3 
>> headers
>> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
>>
>> Perhaps you will need to change audit_ip4/6 to return "false" when it can't
>> get the l3 information now so we only log zero addresses when the packet
>> really did contain them.
>
> Ok, to clarify the implications, are you saying that handing a NULL
> pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"

My initial reaction is that if the packet is so badly
truncated/malformed that we don't have a full IP header than we should
just refrain from logging the packet; it's too malformed/garbage to
offer any useful information and the normal packet processing should
result in the packet being discarded anyway.

-- 
paul moore
www.paul-moore.com
--
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 0/8] Netfilter fixes for net

2017-02-23 Thread David Miller 
From: Pablo Neira Ayuso 
Date: Thu, 23 Feb 2017 12:14:01 +0100

> The following patchset contains Netfilter fixes for your net tree,
> they are:
 ...
> You can pull these changes from:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git

Pulled, thanks a lot!
--
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 V2] audit: normalize NETFILTER_PKT

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 06:20, Florian Westphal wrote:
> Richard Guy Briggs  wrote:
> > Simplify and eliminate flipping in and out of message fields, relying on 
> > nfmark
> > the way we do for audit_key.
> > 
> > +struct nfpkt_par {
> > +   int ipv;
> > +   const void *saddr;
> > +   const void *daddr;
> > +   u8 proto;
> > +};
> 
> This is problematic, see below for why.
> 
> > -static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb)
> > +static void audit_ip4(struct audit_buffer *ab, struct sk_buff *skb, struct 
> > nfpkt_par *apar)
> >  {
> > struct iphdr _iph;
> > const struct iphdr *ih;
> >  
> > +   apar->ipv = 4;
> > ih = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> > -   if (!ih) {
> > -   audit_log_format(ab, " truncated=1");
> > +   if (!ih)
> > return;
> 
> Removing this "truncated" has the consequence that this can later log
> "saddr=0.0.0.0 daddr=0.0.0.0" if we return here.
> 
> This cannot happen for ip(6)tables because ip stack discards broken l3 headers
> before the netfilter hooks get called, but its possible with NFPROTO_BRIDGE.
> 
> Perhaps you will need to change audit_ip4/6 to return "false" when it can't
> get the l3 information now so we only log zero addresses when the packet
> really did contain them.

Ok, to clarify the implications, are you saying that handing a NULL
pointer to "saddr=%pI4" will print "0.0.0.0" rather than "(none)" or "?"

> > -   audit_log_format(ab, " saddr=%pI4 daddr=%pI4 ipid=%hu proto=%hhu",
> > -   >saddr, >daddr, ntohs(ih->id), ih->protocol);
> 
> Alternatively, one could keep this around. In fact, why is this (re)moved
> in first place?  This move of audit_log_format() seems to only reason
> why *apar struct is required.
> 
> AFAICS this now does:
>   ab = new()
>   log(ab, mark);
>   audit_ip4();
>   log();
> 
> so might as well keep the log() call within the audit_ip4/6 function.

Understood.  The apar parameter was conceived for the previous patch
with 20 fields and made more sense then.

> > -   audit_proto(ab, skb, ih->protocol, ih->ihl * 4);
> > +   apar->saddr = >saddr;
> > +   apar->daddr = >daddr;
> > +   apar->proto = ih->protocol;
> >  }
> 
> Caution.  skb_header_pointer() may copy from non-linear skb part
> into _iph, which is on stack, so apar->saddr may be stale once
> function returns. So if you really want to remove the audit_log_format()
> of the saddr/daddr then you need to copy the ip addresses here.
> 
> (We guarantee its linear for ip stack but not for NFPROTO_BRIDGE and this 
> function
> is also called for the bridge version of the target).

Ok, all the more reason to keep the log call in the protocol family function 
call.

> >  static unsigned int
> >  audit_tg(struct sk_buff *skb, const struct xt_action_param *par)
> >  {
> > -   const struct xt_audit_info *info = par->targinfo;
> > struct audit_buffer *ab;
> > +   struct nfpkt_par apar = {
> > +   -1, NULL, NULL, -1,
> > +   };
> 
> I suggest to use
>   struct nfpkt_par apar = {
>   .family = par->family,
>   };
> 
> if apar is required for some reason.

I did look at this originally, then realized that netfilter doesn't use
the same protocol family identifiers as standard ethernet headers that
are used in the bridge case or IP protocol or IPv6 next header.  If I
were to pick one, I might use the ethernet header conventions for next
protocol (ethertype, except they are 16 bits instead of 8).

> > ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > if (ab == NULL)
> > goto errout;
> >  
> > -   audit_log_format(ab, "action=%hhu hook=%u len=%u inif=%s outif=%s",
> > -info->type, par->hooknum, skb->len,
> > -par->in ? par->in->name : "?",
> > -par->out ? par->out->name : "?");
> > -
> > -   if (skb->mark)
> > -   audit_log_format(ab, " mark=%#x", skb->mark);
> > +   audit_log_format(ab, " mark=%#x", skb->mark ?: -1);
> 
> -1 will be logged as 0x, no?  whats wrong with
>   audit_log_format(ab, " mark=%#x", skb->mark); ?

You are correct, this was hasty.

> > if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
> > -   audit_log_format(ab, " smac=%pM dmac=%pM macproto=0x%04x",
> > -eth_hdr(skb)->h_source, eth_hdr(skb)->h_dest,
> > -ntohs(eth_hdr(skb)->h_proto));
> > -
> 
> This ARPHDR_ETHER test is no longer needed after logging
> of ether addresses is removed.

Ok, that helps simplify the first level logic.  I was really hoping to
keep "macproto" to make clear what protocol family was in play, but
that's when I noticed that par->family doesn't use a standard set of
numbers I recognize.  This isn't a problem because this can be indicated
by a small number of bits in the nfmark.


Thanks for your quick review.

- RGB

--
Richard Guy Briggs 
Kernel Security Engineering, Base Operating Systems,

Re: [PATCH V2] audit: normalize NETFILTER_PKT (fwd)

2017-02-23 Thread Richard Guy Briggs 
On 2017-02-23 08:12, Julia Lawall wrote:
> Hello,
> 
> It looks like the switch starting on line 106 should be indented more if
> it is expected to be under the if in line 105.  I believe that there
> should also be braces around the switch.  It is a single statement, but it
> is a complex one.

Yes, that switch statement should be indented, brace-to-brace.  If the
entire switch is indented including its own braces, the external braces
around the switch should be unnecessary.

Thanks.

> thanks,
> julia
> 
> -- Forwarded message --
> Date: Thu, 23 Feb 2017 12:43:05 +0800
> From: kbuild test robot <fengguang...@intel.com>
> To: kbu...@01.org
> Cc: Julia Lawall <julia.law...@lip6.fr>
> Subject: Re: [PATCH V2] audit: normalize NETFILTER_PKT
> 
> CC: kbuild-...@01.org
> In-Reply-To: 
> <9504740e9333a0b7074abe0dddfc487aeeae6cff.1487813996.git@redhat.com>
> 
> Hi Richard,
> 
> [auto build test WARNING on v4.9-rc8]
> [cannot apply to nf-next/master next-20170222]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:    
> https://github.com/0day-ci/linux/commits/Richard-Guy-Briggs/audit-normalize-NETFILTER_PKT/20170223-110223
> :: branch date: 2 hours ago
> :: commit date: 2 hours ago
> 
> >> net/netfilter/xt_AUDIT.c:106:1-2: code aligned with following code on line 
> >> 116
> 
> git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 8f27486f1987d344c4d9b0de556dfd4209c524bf
> vim +106 net/netfilter/xt_AUDIT.c
> 
> 8f27486f Richard Guy Briggs 2017-02-22  100   
> audit_ip6(ab, skb, );
> 43f393ca Thomas Graf2011-01-16  101   break;
> 43f393ca Thomas Graf2011-01-16  102   }
> 43f393ca Thomas Graf2011-01-16  103   }
> 43f393ca Thomas Graf2011-01-16  104   }
> 8f27486f Richard Guy Briggs 2017-02-22  105   if (apar.ipv == -1)
> 43f393ca Thomas Graf2011-01-16 @106   switch (par->family) {
> 43f393ca Thomas Graf2011-01-16  107   case NFPROTO_IPV4:
> 8f27486f Richard Guy Briggs 2017-02-22  108   audit_ip4(ab, skb, 
> );
> 43f393ca Thomas Graf2011-01-16  109   break;
> 43f393ca Thomas Graf2011-01-16  110
> 43f393ca Thomas Graf2011-01-16  111   case NFPROTO_IPV6:
> 8f27486f Richard Guy Briggs 2017-02-22  112   audit_ip6(ab, skb, 
> );
> 43f393ca Thomas Graf2011-01-16  113   break;
> 43f393ca Thomas Graf2011-01-16  114   }
> 43f393ca Thomas Graf2011-01-16  115
> 8f27486f Richard Guy Briggs 2017-02-22 @116   switch (apar.ipv) {
> 8f27486f Richard Guy Briggs 2017-02-22  117   case 4:
> 8f27486f Richard Guy Briggs 2017-02-22  118   audit_log_format(ab, " 
> saddr=%pI4 daddr=%pI4 proto=%hhu",
> 8f27486f Richard Guy Briggs 2017-02-22  119apar.saddr, 
> apar.daddr, apar.proto);
> 
> :: The code at line 106 was first introduced by commit
> :: 43f393caec0362abe03c72799d3f342af3973070 netfilter: audit target to 
> record accepted/dropped packets
> 
> :: TO: Thomas Graf <tg...@infradead.org>
> :: CC: Patrick McHardy <ka...@trash.net>
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation

- RGB

--
Richard Guy Briggs <r...@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635
--
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-next 2/2] netfilter: nft_hash: support of symmetric hash

2017-02-23 Thread kbuild test robot 
Hi Laura,

[auto build test WARNING on v4.9-rc8]
[cannot apply to next-20170223]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Laura-Garcia-Liebana/netfilter-nft_hash-symhash-type-support/20170223-205609
config: i386-randconfig-x018-201708 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

   net/netfilter/nft_hash.c: In function 'nft_symhash_eval':
>> net/netfilter/nft_hash.c:55:48: warning: passing argument 1 of 
>> '__skb_get_hash_symmetric' discards 'const' qualifier from pointer target 
>> type [-Wdiscarded-qualifiers]
 h = reciprocal_scale(__skb_get_hash_symmetric(skb), priv->modulus);
   ^~~
   In file included from include/linux/netlink.h:6:0,
from net/netfilter/nft_hash.c:13:
   include/linux/skbuff.h:1090:5: note: expected 'struct sk_buff *' but 
argument is of type 'const struct sk_buff *'
u32 __skb_get_hash_symmetric(struct sk_buff *skb);
^~~~

vim +55 net/netfilter/nft_hash.c

39  }
40  
41  struct nft_symhash {
42  enum nft_registers  dreg:8;
43  u32 modulus;
44  u32 offset;
45  };
46  
47  static void nft_symhash_eval(const struct nft_expr *expr,
48   struct nft_regs *regs,
49   const struct nft_pktinfo *pkt)
50  {
51  struct nft_symhash *priv = nft_expr_priv(expr);
52  const struct sk_buff *skb = pkt->skb;
53  u32 h;
54  
  > 55  h = reciprocal_scale(__skb_get_hash_symmetric(skb), 
priv->modulus);
56  
57  regs->data[priv->dreg] = h + priv->offset;
58  }
59  
60  static const struct nla_policy nft_hash_policy[NFTA_HASH_MAX + 1] = {
61  [NFTA_HASH_SREG]= { .type = NLA_U32 },
62  [NFTA_HASH_DREG]= { .type = NLA_U32 },
63  [NFTA_HASH_LEN] = { .type = NLA_U32 },

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: netfilter: nft_ct: add zone id set support

2017-02-23 Thread Florian Westphal 
Pablo Neira Ayuso  wrote:
> On Thu, Feb 23, 2017 at 12:34:35PM +0100, Florian Westphal wrote:
> > Yes, Dan reported this and a patch is queued at
> > http://patchwork.ozlabs.org/patch/727573/
> > 
> > Pablo, any reason why this is still waiting?
> 
> I just flushing out my nf.git tree via pull request.
> 
> Once these changes are pulled, I'll fetch recent net-next changes that
> were just merged via net. Then, I'll pick this so we can calm down
> these compilation warnings.
> 
> Are you OK with this procedure? Thanks!

Sure.
--
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: netfilter: nft_ct: add zone id set support

2017-02-23 Thread Pablo Neira Ayuso 
On Thu, Feb 23, 2017 at 12:34:35PM +0100, Florian Westphal wrote:
> Geert Uytterhoeven  wrote:
> > On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
> >  wrote:
> > > Web:
> > > https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> > > Commit: edee4f1e92458299505ff007733f676b00c516a1
> > > Parent: 5c178d81b69f08ca3195427a6ea9a46d9af23127
> > > Refname:refs/heads/master
> > > Author: Florian Westphal 
> > > AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> > > Committer:  Pablo Neira Ayuso 
> > > CommitDate: Wed Feb 8 14:16:23 2017 +0100
> > >
> > Unlike for the other cases of the switch statement, "len" is not initialized
> > here...
> > 
> > > +   break;
> > > priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]);
> > > err = nft_validate_register_load(priv->sreg, len);
> > 
> > ... and used here, which may lead to spurious failures of
> > nft_validate_register_load().
> 
> Yes, Dan reported this and a patch is queued at
> http://patchwork.ozlabs.org/patch/727573/
> 
> Pablo, any reason why this is still waiting?

I just flushing out my nf.git tree via pull request.

Once these changes are pulled, I'll fetch recent net-next changes that
were just merged via net. Then, I'll pick this so we can calm down
these compilation warnings.

Are you OK with this procedure? Thanks!
--
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: netfilter: nft_ct: add zone id set support

2017-02-23 Thread Florian Westphal 
Geert Uytterhoeven  wrote:
> On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
>  wrote:
> > Web:
> > https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> > Commit: edee4f1e92458299505ff007733f676b00c516a1
> > Parent: 5c178d81b69f08ca3195427a6ea9a46d9af23127
> > Refname:refs/heads/master
> > Author: Florian Westphal 
> > AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> > Committer:  Pablo Neira Ayuso 
> > CommitDate: Wed Feb 8 14:16:23 2017 +0100
> >
> Unlike for the other cases of the switch statement, "len" is not initialized
> here...
> 
> > +   break;
> > priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]);
> > err = nft_validate_register_load(priv->sreg, len);
> 
> ... and used here, which may lead to spurious failures of
> nft_validate_register_load().

Yes, Dan reported this and a patch is queued at
http://patchwork.ozlabs.org/patch/727573/

Pablo, any reason why this is still waiting?
Do you want me to run more tests?

--
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

[PATCH nft] src: hash: support of symmetric hash

2017-02-23 Thread Laura Garcia Liebana 
This patch provides symmetric hash support according to source
ip address and port, and destination ip address and port.

The new attribute NFTA_HASH_TYPE has been included to support
different types of hashing functions. Currently supported
NFT_HASH_JENKINS through jhash and NFT_HASH_SYM through symhash.

The main difference between both types are:
 - jhash requires an expression with sreg, symhash doesn't.
 - symhash supports modulus and offset, but not seed.

Examples:

 nft add rule ip nat prerouting ct mark set jhash ip saddr mod 2
 nft add rule ip nat prerouting ct mark set symhash mod 2

Signed-off-by: Laura Garcia Liebana 
---
 include/expression.h|  1 +
 include/hash.h  |  2 +-
 include/linux/netfilter/nf_tables.h | 13 +
 src/evaluate.c  |  3 ++-
 src/hash.c  | 28 +---
 src/netlink_delinearize.c   | 35 +--
 src/netlink_linearize.c | 20 
 src/parser_bison.y  | 16 ++--
 src/scanner.l   |  1 +
 tests/py/ip/hash.t  |  1 +
 tests/py/ip/hash.t.payload  |  4 
 11 files changed, 87 insertions(+), 37 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index ec90265..56cb310 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -308,6 +308,7 @@ struct expr {
uint32_tmod;
uint32_tseed;
uint32_toffset;
+   enum nft_hash_types type;
} hash;
struct {
/* EXPR_FIB */
diff --git a/include/hash.h b/include/hash.h
index 8bf53e2..7f9c6f1 100644
--- a/include/hash.h
+++ b/include/hash.h
@@ -3,6 +3,6 @@
 
 extern struct expr *hash_expr_alloc(const struct location *loc,
uint32_t modulus, uint32_t seed,
-   uint32_t offset);
+   uint32_t offset, enum nft_hash_types type);
 
 #endif /* NFTABLES_HASH_H */
diff --git a/include/linux/netfilter/nf_tables.h 
b/include/linux/netfilter/nf_tables.h
index b00a05d..74a42fa 100644
--- a/include/linux/netfilter/nf_tables.h
+++ b/include/linux/netfilter/nf_tables.h
@@ -793,6 +793,17 @@ enum nft_rt_keys {
 };
 
 /**
+ * enum nft_hash_types - nf_tables hash expression types
+ *
+ * @NFT_HASH_JENKINS: Jenkins Hash
+ * @NFT_HASH_SYM: Symmetric Hash
+ */
+enum nft_hash_types {
+   NFT_HASH_JENKINS,
+   NFT_HASH_SYM,
+};
+
+/**
  * enum nft_hash_attributes - nf_tables hash expression netlink attributes
  *
  * @NFTA_HASH_SREG: source register (NLA_U32)
@@ -801,6 +812,7 @@ enum nft_rt_keys {
  * @NFTA_HASH_MODULUS: modulus value (NLA_U32)
  * @NFTA_HASH_SEED: seed value (NLA_U32)
  * @NFTA_HASH_OFFSET: add this offset value to hash result (NLA_U32)
+ * @NFTA_HASH_TYPE: hash operation (NLA_U32: nft_hash_types)
  */
 enum nft_hash_attributes {
NFTA_HASH_UNSPEC,
@@ -810,6 +822,7 @@ enum nft_hash_attributes {
NFTA_HASH_MODULUS,
NFTA_HASH_SEED,
NFTA_HASH_OFFSET,
+   NFTA_HASH_TYPE,
__NFTA_HASH_MAX,
 };
 #define NFTA_HASH_MAX  (__NFTA_HASH_MAX - 1)
diff --git a/src/evaluate.c b/src/evaluate.c
index 94412f2..18884be 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1249,7 +1249,8 @@ static int expr_evaluate_hash(struct eval_ctx *ctx, 
struct expr **exprp)
expr_dtype_integer_compatible(ctx, expr);
 
expr_set_context(>ectx, NULL, 0);
-   if (expr_evaluate(ctx, >hash.expr) < 0)
+   if (expr->hash.expr &&
+   expr_evaluate(ctx, >hash.expr) < 0)
return -1;
 
/* expr_evaluate_primary() sets the context to what to the input
diff --git a/src/hash.c b/src/hash.c
index d26b2ed..a7a9612 100644
--- a/src/hash.c
+++ b/src/hash.c
@@ -17,10 +17,18 @@
 
 static void hash_expr_print(const struct expr *expr)
 {
-   printf("jhash ");
-   expr_print(expr->hash.expr);
+   switch (expr->hash.type) {
+   case NFT_HASH_SYM:
+   printf("symhash");
+   break;
+   case NFT_HASH_JENKINS:
+   default:
+   printf("jhash ");
+   expr_print(expr->hash.expr);
+   }
+
printf(" mod %u", expr->hash.mod);
-   if (expr->hash.seed)
+   if (expr->hash.type & NFT_HASH_JENKINS && expr->hash.seed)
printf(" seed 0x%x", expr->hash.seed);
if (expr->hash.offset)
printf(" offset %u", expr->hash.offset);
@@ -28,18 +36,22 @@ static void hash_expr_print(const struct expr *expr)
 
 static bool hash_expr_cmp(const struct expr *e1, const struct expr *e2)
 {
-   return expr_cmp(e1->hash.expr, e2->hash.expr) &&
+   return (e1->hash.expr ||
+   expr_cmp(e1->hash.expr, e2->hash.expr)) &&

[PATCH nf-next 2/2] netfilter: nft_hash: support of symmetric hash

2017-02-23 Thread Laura Garcia Liebana 
This patch provides symmetric hash support according to source
ip address and port, and destination ip address and port.

For this purpose, the __skb_get_hash_symmetric() is used to
identify the flow as it uses FLOW_DISSECTOR_F_STOP_AT_FLOW_LABEL
flag by default.

The new attribute NFTA_HASH_TYPE has been included to support
different types of hashing functions. Currently supported
NFT_HASH_JENKINS through jhash and NFT_HASH_SYM through symhash.

The main difference between both types are:
 - jhash requires an expression with sreg, symhash doesn't.
 - symhash supports modulus and offset, but not seed.

Examples:

 nft add rule ip nat prerouting ct mark set jhash ip saddr mod 2
 nft add rule ip nat prerouting ct mark set symhash mod 2

Signed-off-by: Laura Garcia Liebana 
---
 include/uapi/linux/netfilter/nf_tables.h | 13 +
 net/netfilter/nft_hash.c | 98 +++-
 2 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nf_tables.h 
b/include/uapi/linux/netfilter/nf_tables.h
index 7b730cab99bd..a444a63a9eee 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -793,6 +793,17 @@ enum nft_rt_keys {
 };
 
 /**
+ * enum nft_hash_types - nf_tables hash expression types
+ *
+ * @NFT_HASH_JENKINS: Jenkins Hash
+ * @NFT_HASH_SYM: Symmetric Hash
+ */
+enum nft_hash_types {
+   NFT_HASH_JENKINS,
+   NFT_HASH_SYM,
+};
+
+/**
  * enum nft_hash_attributes - nf_tables hash expression netlink attributes
  *
  * @NFTA_HASH_SREG: source register (NLA_U32)
@@ -801,6 +812,7 @@ enum nft_rt_keys {
  * @NFTA_HASH_MODULUS: modulus value (NLA_U32)
  * @NFTA_HASH_SEED: seed value (NLA_U32)
  * @NFTA_HASH_OFFSET: add this offset value to hash result (NLA_U32)
+ * @NFTA_HASH_TYPE: hash operation (NLA_U32: nft_hash_types)
  */
 enum nft_hash_attributes {
NFTA_HASH_UNSPEC,
@@ -810,6 +822,7 @@ enum nft_hash_attributes {
NFTA_HASH_MODULUS,
NFTA_HASH_SEED,
NFTA_HASH_OFFSET,
+   NFTA_HASH_TYPE,
__NFTA_HASH_MAX,
 };
 #define NFTA_HASH_MAX  (__NFTA_HASH_MAX - 1)
diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index ccb834ef049b..da0171908f92 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -38,6 +38,25 @@ static void nft_jhash_eval(const struct nft_expr *expr,
regs->data[priv->dreg] = h + priv->offset;
 }
 
+struct nft_symhash {
+   enum nft_registers  dreg:8;
+   u32 modulus;
+   u32 offset;
+};
+
+static void nft_symhash_eval(const struct nft_expr *expr,
+struct nft_regs *regs,
+const struct nft_pktinfo *pkt)
+{
+   struct nft_symhash *priv = nft_expr_priv(expr);
+   const struct sk_buff *skb = pkt->skb;
+   u32 h;
+
+   h = reciprocal_scale(__skb_get_hash_symmetric(skb), priv->modulus);
+
+   regs->data[priv->dreg] = h + priv->offset;
+}
+
 static const struct nla_policy nft_hash_policy[NFTA_HASH_MAX + 1] = {
[NFTA_HASH_SREG]= { .type = NLA_U32 },
[NFTA_HASH_DREG]= { .type = NLA_U32 },
@@ -45,6 +64,7 @@ static const struct nla_policy nft_hash_policy[NFTA_HASH_MAX 
+ 1] = {
[NFTA_HASH_MODULUS] = { .type = NLA_U32 },
[NFTA_HASH_SEED]= { .type = NLA_U32 },
[NFTA_HASH_OFFSET]  = { .type = NLA_U32 },
+   [NFTA_HASH_TYPE]= { .type = NLA_U32 },
 };
 
 static int nft_jhash_init(const struct nft_ctx *ctx,
@@ -92,6 +112,32 @@ static int nft_jhash_init(const struct nft_ctx *ctx,
   NFT_DATA_VALUE, sizeof(u32));
 }
 
+static int nft_symhash_init(const struct nft_ctx *ctx,
+   const struct nft_expr *expr,
+   const struct nlattr * const tb[])
+{
+   struct nft_symhash *priv = nft_expr_priv(expr);
+
+   if (!tb[NFTA_HASH_DREG]||
+   !tb[NFTA_HASH_MODULUS])
+   return -EINVAL;
+
+   if (tb[NFTA_HASH_OFFSET])
+   priv->offset = ntohl(nla_get_be32(tb[NFTA_HASH_OFFSET]));
+
+   priv->dreg = nft_parse_register(tb[NFTA_HASH_DREG]);
+
+   priv->modulus = ntohl(nla_get_be32(tb[NFTA_HASH_MODULUS]));
+   if (priv->modulus <= 1)
+   return -ERANGE;
+
+   if (priv->offset + priv->modulus - 1 < priv->offset)
+   return -EOVERFLOW;
+
+   return nft_validate_register_store(ctx, priv->dreg, NULL,
+  NFT_DATA_VALUE, sizeof(u32));
+}
+
 static int nft_jhash_dump(struct sk_buff *skb,
  const struct nft_expr *expr)
 {
@@ -110,6 +156,28 @@ static int nft_jhash_dump(struct sk_buff *skb,
if (priv->offset != 0)
if (nla_put_be32(skb, NFTA_HASH_OFFSET, htonl(priv->offset)))
goto nla_put_failure;
+   if (nla_put_be32(skb,

[PATCH nf-next 1/2] netfilter: nft_hash: rename nft_hash to nft_jhash

2017-02-23 Thread Laura Garcia Liebana 
This patch renames the local nft_hash structure and functions
to nft_jhash in order to prepare the nft_hash module code to
add new hash functions.

Signed-off-by: Laura Garcia Liebana 
---
 net/netfilter/nft_hash.c | 36 ++--
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/net/netfilter/nft_hash.c b/net/netfilter/nft_hash.c
index eb2721af898d..ccb834ef049b 100644
--- a/net/netfilter/nft_hash.c
+++ b/net/netfilter/nft_hash.c
@@ -17,7 +17,7 @@
 #include 
 #include 
 
-struct nft_hash {
+struct nft_jhash {
enum nft_registers  sreg:8;
enum nft_registers  dreg:8;
u8  len;
@@ -26,11 +26,11 @@ struct nft_hash {
u32 offset;
 };
 
-static void nft_hash_eval(const struct nft_expr *expr,
- struct nft_regs *regs,
- const struct nft_pktinfo *pkt)
+static void nft_jhash_eval(const struct nft_expr *expr,
+  struct nft_regs *regs,
+  const struct nft_pktinfo *pkt)
 {
-   struct nft_hash *priv = nft_expr_priv(expr);
+   struct nft_jhash *priv = nft_expr_priv(expr);
const void *data = >data[priv->sreg];
u32 h;
 
@@ -47,11 +47,11 @@ static const struct nla_policy 
nft_hash_policy[NFTA_HASH_MAX + 1] = {
[NFTA_HASH_OFFSET]  = { .type = NLA_U32 },
 };
 
-static int nft_hash_init(const struct nft_ctx *ctx,
-const struct nft_expr *expr,
-const struct nlattr * const tb[])
+static int nft_jhash_init(const struct nft_ctx *ctx,
+ const struct nft_expr *expr,
+ const struct nlattr * const tb[])
 {
-   struct nft_hash *priv = nft_expr_priv(expr);
+   struct nft_jhash *priv = nft_expr_priv(expr);
u32 len;
int err;
 
@@ -92,10 +92,10 @@ static int nft_hash_init(const struct nft_ctx *ctx,
   NFT_DATA_VALUE, sizeof(u32));
 }
 
-static int nft_hash_dump(struct sk_buff *skb,
-const struct nft_expr *expr)
+static int nft_jhash_dump(struct sk_buff *skb,
+ const struct nft_expr *expr)
 {
-   const struct nft_hash *priv = nft_expr_priv(expr);
+   const struct nft_jhash *priv = nft_expr_priv(expr);
 
if (nft_dump_register(skb, NFTA_HASH_SREG, priv->sreg))
goto nla_put_failure;
@@ -117,17 +117,17 @@ static int nft_hash_dump(struct sk_buff *skb,
 }
 
 static struct nft_expr_type nft_hash_type;
-static const struct nft_expr_ops nft_hash_ops = {
+static const struct nft_expr_ops nft_jhash_ops = {
.type   = _hash_type,
-   .size   = NFT_EXPR_SIZE(sizeof(struct nft_hash)),
-   .eval   = nft_hash_eval,
-   .init   = nft_hash_init,
-   .dump   = nft_hash_dump,
+   .size   = NFT_EXPR_SIZE(sizeof(struct nft_jhash)),
+   .eval   = nft_jhash_eval,
+   .init   = nft_jhash_init,
+   .dump   = nft_jhash_dump,
 };
 
 static struct nft_expr_type nft_hash_type __read_mostly = {
.name   = "hash",
-   .ops= _hash_ops,
+   .ops= _jhash_ops,
.policy = nft_hash_policy,
.maxattr= NFTA_HASH_MAX,
.owner  = THIS_MODULE,
-- 
2.11.0

--
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

[PATCH 1/8] netfilter: nf_ct_helper: warn when not applying default helper assignment

2017-02-23 Thread Pablo Neira Ayuso 
From: Jiri Kosina 

Commit 3bb398d925 ("netfilter: nf_ct_helper: disable automatic helper
assignment") is causing behavior regressions in firewalls, as traffic
handled by conntrack helpers is now by default not passed through even
though it was before due to missing CT targets (which were not necessary
before this commit).

The default had to be switched off due to security reasons [1] [2] and
therefore should stay the way it is, but let's be friendly to firewall
admins and issue a warning the first time we're in situation where packet
would be likely passed through with the old default but we're likely going
to drop it on the floor now.

Rewrite the code a little bit as suggested by Linus, so that we avoid
spaghettiing the code even more -- namely the whole decision making
process regarding helper selection (either automatic or not) is being
separated, so that the whole logic can be simplified and code (condition)
duplication reduced.

[1] https://cansecwest.com/csw12/conntrack-attack.pdf
[2] https://home.regit.org/netfilter-en/secure-use-of-helpers/

Signed-off-by: Jiri Kosina 
Signed-off-by: Pablo Neira Ayuso 
---
 net/netfilter/nf_conntrack_helper.c | 39 -
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/net/netfilter/nf_conntrack_helper.c 
b/net/netfilter/nf_conntrack_helper.c
index 7341adf7059d..6dc44d9b4190 100644
--- a/net/netfilter/nf_conntrack_helper.c
+++ b/net/netfilter/nf_conntrack_helper.c
@@ -188,6 +188,26 @@ nf_ct_helper_ext_add(struct nf_conn *ct,
 }
 EXPORT_SYMBOL_GPL(nf_ct_helper_ext_add);
 
+static struct nf_conntrack_helper *
+nf_ct_lookup_helper(struct nf_conn *ct, struct net *net)
+{
+   if (!net->ct.sysctl_auto_assign_helper) {
+   if (net->ct.auto_assign_helper_warned)
+   return NULL;
+   if (!__nf_ct_helper_find(>tuplehash[IP_CT_DIR_REPLY].tuple))
+   return NULL;
+   pr_info("nf_conntrack: default automatic helper assignment "
+   "has been turned off for security reasons and CT-based "
+   " firewall rule not found. Use the iptables CT target "
+   "to attach helpers instead.\n");
+   net->ct.auto_assign_helper_warned = 1;
+   return NULL;
+   }
+
+   return __nf_ct_helper_find(>tuplehash[IP_CT_DIR_REPLY].tuple);
+}
+
+
 int __nf_ct_try_assign_helper(struct nf_conn *ct, struct nf_conn *tmpl,
  gfp_t flags)
 {
@@ -213,21 +233,14 @@ int __nf_ct_try_assign_helper(struct nf_conn *ct, struct 
nf_conn *tmpl,
}
 
help = nfct_help(ct);
-   if (net->ct.sysctl_auto_assign_helper && helper == NULL) {
-   helper = 
__nf_ct_helper_find(>tuplehash[IP_CT_DIR_REPLY].tuple);
-   if (unlikely(!net->ct.auto_assign_helper_warned && helper)) {
-   pr_info("nf_conntrack: automatic helper "
-   "assignment is deprecated and it will "
-   "be removed soon. Use the iptables CT target "
-   "to attach helpers instead.\n");
-   net->ct.auto_assign_helper_warned = true;
-   }
-   }
 
if (helper == NULL) {
-   if (help)
-   RCU_INIT_POINTER(help->helper, NULL);
-   return 0;
+   helper = nf_ct_lookup_helper(ct, net);
+   if (helper == NULL) {
+   if (help)
+   RCU_INIT_POINTER(help->helper, NULL);
+   return 0;
+   }
}
 
if (help == NULL) {
-- 
2.1.4

--
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

[PATCH 3/8] netfilter: ctnetlink: Fix regression in CTA_HELP processing

2017-02-23 Thread Pablo Neira Ayuso 
From: Kevin Cernekee 

Prior to Linux 4.4, it was usually harmless to send a CTA_HELP attribute
containing the name of the current helper.  That is no longer the case:
as of Linux 4.4, if ctnetlink_change_helper() returns an error from
the ct->master check, processing of the request will fail, skipping the
NFQA_EXP attribute (if present).

This patch changes the behavior to improve compatibility with user
programs that expect the kernel interface to work the way it did prior
to Linux 4.4.  If a user program specifies CTA_HELP but the argument
matches the current conntrack helper name, ignore it instead of generating
an error.

Signed-off-by: Kevin Cernekee 
Signed-off-by: Pablo Neira Ayuso 
---
 net/netfilter/nf_conntrack_netlink.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index bf04b7e9d6f7..6806b5e73567 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1478,14 +1478,23 @@ static int ctnetlink_change_helper(struct nf_conn *ct,
struct nlattr *helpinfo = NULL;
int err;
 
-   /* don't change helper of sibling connections */
-   if (ct->master)
-   return -EBUSY;
-
err = ctnetlink_parse_help(cda[CTA_HELP], , );
if (err < 0)
return err;
 
+   /* don't change helper of sibling connections */
+   if (ct->master) {
+   /* If we try to change the helper to the same thing twice,
+* treat the second attempt as a no-op instead of returning
+* an error.
+*/
+   if (help && help->helper &&
+   !strcmp(help->helper->name, helpname))
+   return 0;
+   else
+   return -EBUSY;
+   }
+
if (!strcmp(helpname, "")) {
if (help && help->helper) {
/* we had a helper before ... */
-- 
2.1.4

--
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

[PATCH 2/8] netfilter: ctnetlink: Fix regression in CTA_STATUS processing

2017-02-23 Thread Pablo Neira Ayuso 
From: Kevin Cernekee 

The libnetfilter_conntrack userland library always sets IPS_CONFIRMED
when building a CTA_STATUS attribute.  If this toggles the bit from
0->1, the parser will return an error.  On Linux 4.4+ this will cause any
NFQA_EXP attribute in the packet to be ignored.  This breaks conntrackd's
userland helpers because they operate on unconfirmed connections.

Instead of returning -EBUSY if the user program asks to modify an
unchangeable bit, simply ignore the change.

Also, fix the logic so that user programs are allowed to clear
the bits that they are allowed to change.

Signed-off-by: Kevin Cernekee 
Signed-off-by: Pablo Neira Ayuso 
---
 include/uapi/linux/netfilter/nf_conntrack_common.h |  4 
 net/netfilter/nf_conntrack_netlink.c   | 26 +-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nf_conntrack_common.h 
b/include/uapi/linux/netfilter/nf_conntrack_common.h
index 6d074d14ee27..6a8e33dd4ecb 100644
--- a/include/uapi/linux/netfilter/nf_conntrack_common.h
+++ b/include/uapi/linux/netfilter/nf_conntrack_common.h
@@ -82,6 +82,10 @@ 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),
diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index 27540455dc62..bf04b7e9d6f7 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -2270,6 +2270,30 @@ ctnetlink_glue_build(struct sk_buff *skb, struct nf_conn 
*ct,
 }
 
 static int
+ctnetlink_update_status(struct nf_conn *ct, const struct nlattr * const cda[])
+{
+   unsigned int status = ntohl(nla_get_be32(cda[CTA_STATUS]));
+   unsigned long d = ct->status ^ status;
+
+   if (d & IPS_SEEN_REPLY && !(status & IPS_SEEN_REPLY))
+   /* SEEN_REPLY bit can only be set */
+   return -EBUSY;
+
+   if (d & IPS_ASSURED && !(status & IPS_ASSURED))
+   /* ASSURED bit can only be set */
+   return -EBUSY;
+
+   /* This check is less strict than ctnetlink_change_status()
+* because callers often flip IPS_EXPECTED bits when sending
+* an NFQA_CT attribute to the kernel.  So ignore the
+* unchangeable bits but do not error out.
+*/
+   ct->status = (status & ~IPS_UNCHANGEABLE_MASK) |
+(ct->status & IPS_UNCHANGEABLE_MASK);
+   return 0;
+}
+
+static int
 ctnetlink_glue_parse_ct(const struct nlattr *cda[], struct nf_conn *ct)
 {
int err;
@@ -2280,7 +2304,7 @@ ctnetlink_glue_parse_ct(const struct nlattr *cda[], 
struct nf_conn *ct)
return err;
}
if (cda[CTA_STATUS]) {
-   err = ctnetlink_change_status(ct, cda);
+   err = ctnetlink_update_status(ct, cda);
if (err < 0)
return err;
}
-- 
2.1.4

--
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

[PATCH 6/8] netfilter: xt_hashlimit: Fix integer divide round to zero.

2017-02-23 Thread Pablo Neira Ayuso 
From: Alban Browaeys 

Diving the divider by the multiplier before applying to the input.
When this would "divide by zero", divide the multiplier by the divider
first then multiply the input by this value.

Currently user2creds outputs zero when input value is bigger than the
number of slices and  lower than scale.
This as then user input is applied an integer divide operation to
a number greater than itself (scale).
That rounds up to zero, then we multiply zero by the credits slice size.

  iptables -t filter -I INPUT --protocol tcp --match hashlimit
  --hashlimit 40/second --hashlimit-burst 20 --hashlimit-mode srcip
  --hashlimit-name syn-flood --jump RETURN

thus trigger the overflow detection code:

xt_hashlimit: overflow, try lower: 25000/20

(25000 as hashlimit avg and 20 the burst)

Here:
134217 slices of (HZ * CREDITS_PER_JIFFY) size.
50 is user input value
100 is XT_HASHLIMIT_SCALE_v2
gives: 0 as user2creds output
Setting burst to "1" typically solve the issue ...
but setting it to "40" does too !

This is on 32bit arch calling into revision 2 of hashlimit.

Signed-off-by: Alban Browaeys 
Signed-off-by: Pablo Neira Ayuso 
---
 net/netfilter/xt_hashlimit.c | 25 +
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/net/netfilter/xt_hashlimit.c b/net/netfilter/xt_hashlimit.c
index 10063408141d..84ad5ab34558 100644
--- a/net/netfilter/xt_hashlimit.c
+++ b/net/netfilter/xt_hashlimit.c
@@ -463,23 +463,16 @@ static u32 xt_hashlimit_len_to_chunks(u32 len)
 /* Precision saver. */
 static u64 user2credits(u64 user, int revision)
 {
-   if (revision == 1) {
-   /* If multiplying would overflow... */
-   if (user > 0x / (HZ*CREDITS_PER_JIFFY_v1))
-   /* Divide first. */
-   return div64_u64(user, XT_HASHLIMIT_SCALE)
-   * HZ * CREDITS_PER_JIFFY_v1;
-
-   return div64_u64(user * HZ * CREDITS_PER_JIFFY_v1,
-XT_HASHLIMIT_SCALE);
-   } else {
-   if (user > 0xULL / (HZ*CREDITS_PER_JIFFY))
-   return div64_u64(user, XT_HASHLIMIT_SCALE_v2)
-   * HZ * CREDITS_PER_JIFFY;
+   u64 scale = (revision == 1) ?
+   XT_HASHLIMIT_SCALE : XT_HASHLIMIT_SCALE_v2;
+   u64 cpj = (revision == 1) ?
+   CREDITS_PER_JIFFY_v1 : CREDITS_PER_JIFFY;
 
-   return div64_u64(user * HZ * CREDITS_PER_JIFFY,
-XT_HASHLIMIT_SCALE_v2);
-   }
+   /* Avoid overflow: divide the constant operands first */
+   if (scale >= HZ * cpj)
+   return div64_u64(user, div64_u64(scale, HZ * cpj));
+
+   return user * div64_u64(HZ * cpj, scale);
 }
 
 static u32 user2credits_byte(u32 user)
-- 
2.1.4

--
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

[PATCH 7/8] netfilter: nfnetlink_queue: fix NFQA_VLAN_MAX definition

2017-02-23 Thread Pablo Neira Ayuso 
From: Ken-ichirou MATSUZAWA 

Should be - 1 as in other _MAX definitions.

Signed-off-by: Ken-ichirou MATSUZAWA 
Acked-by: Florian Westphal 
Signed-off-by: Pablo Neira Ayuso 
---
 include/uapi/linux/netfilter/nfnetlink_queue.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/netfilter/nfnetlink_queue.h 
b/include/uapi/linux/netfilter/nfnetlink_queue.h
index ae30841ff94e..d42f0396fe30 100644
--- a/include/uapi/linux/netfilter/nfnetlink_queue.h
+++ b/include/uapi/linux/netfilter/nfnetlink_queue.h
@@ -36,7 +36,7 @@ enum nfqnl_vlan_attr {
NFQA_VLAN_TCI,  /* __be16 skb htons(vlan_tci) */
__NFQA_VLAN_MAX,
 };
-#define NFQA_VLAN_MAX (__NFQA_VLAN_MAX + 1)
+#define NFQA_VLAN_MAX (__NFQA_VLAN_MAX - 1)
 
 enum nfqnl_attr_type {
NFQA_UNSPEC,
-- 
2.1.4

--
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

[PATCH 4/8] Fix bug: sometimes valid entries in hash:* types of sets were evicted

2017-02-23 Thread Pablo Neira Ayuso 
From: Jozsef Kadlecsik 

Wrong index was used and therefore when shrinking a hash bucket at
deleting an entry, valid entries could be evicted as well.
Thanks to Eric Ewanco for the thorough bugreport.

Fixes netfilter bugzilla #1119

Signed-off-by: Jozsef Kadlecsik 
---
 net/netfilter/ipset/ip_set_hash_gen.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/ipset/ip_set_hash_gen.h 
b/net/netfilter/ipset/ip_set_hash_gen.h
index 1b05d4a7d5a1..f236c0bc7b3f 100644
--- a/net/netfilter/ipset/ip_set_hash_gen.h
+++ b/net/netfilter/ipset/ip_set_hash_gen.h
@@ -897,7 +897,7 @@ mtype_del(struct ip_set *set, void *value, const struct 
ip_set_ext *ext,
continue;
data = ahash_data(n, j, dsize);
memcpy(tmp->value + k * dsize, data, dsize);
-   set_bit(j, tmp->used);
+   set_bit(k, tmp->used);
k++;
}
tmp->pos = k;
-- 
2.1.4

--
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

[PATCH 0/8] Netfilter fixes for net

2017-02-23 Thread Pablo Neira Ayuso 
Hi David,

The following patchset contains Netfilter fixes for your net tree,
they are:

1) Revisit warning logic when not applying default helper assignment.
   Jiri Kosina considers we are breaking existing setups and not warning
   our users accordinly now that automatic helper assignment has been
   turned off by default. So let's make him happy by spotting the warning
   by when we find a helper but we cannot attach, instead of warning on the
   former deprecated behaviour. Patch from Jiri Kosina.

2) Two patches to fix regression in ctnetlink interfaces with
   nfnetlink_queue. Specifically, perform more relaxed in CTA_STATUS
   and do not bail out if CTA_HELP indicates the same helper that we
   already have. Patches from Kevin Cernekee.

3) A couple of bugfixes for ipset via Jozsef Kadlecsik. Due to wrong
   index logic in hash set types and null pointer exception in the
   list:set type.

4) hashlimit bails out with correct userspace parameters due to wrong
   arithmetics in the code that avoids "divide by zero" when
   transforming the userspace timing in milliseconds to token credits.
   Patch from Alban Browaeys.

5) Fix incorrect NFQA_VLAN_MAX definition, patch from
   Ken-ichirou MATSUZAWA.

6) Don't not declare nfnetlink batch error list as static, since this
   may be used by several subsystems at the same time. Patch from
   Liping Zhang.

You can pull these changes from:

  git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git

Thanks!



The following changes since commit cafe8df8b9bc9aa3dffa827c1a6757c6cd36f657:

  net: phy: Fix lack of reference count on PHY driver (2017-02-02 22:59:43 
-0500)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git HEAD

for you to fetch changes up to 3ef767e5cbd405abfd01339c7e5daaf98e037be2:

  Merge branch 'master' of git://blackhole.kfki.hu/nf (2017-02-21 14:01:05 
+0100)


Alban Browaeys (1):
  netfilter: xt_hashlimit: Fix integer divide round to zero.

Jiri Kosina (1):
  netfilter: nf_ct_helper: warn when not applying default helper assignment

Jozsef Kadlecsik (1):
  Fix bug: sometimes valid entries in hash:* types of sets were evicted

Ken-ichirou MATSUZAWA (1):
  netfilter: nfnetlink_queue: fix NFQA_VLAN_MAX definition

Kevin Cernekee (2):
  netfilter: ctnetlink: Fix regression in CTA_STATUS processing
  netfilter: ctnetlink: Fix regression in CTA_HELP processing

Liping Zhang (1):
  netfilter: nfnetlink: remove static declaration from err_list

Pablo Neira Ayuso (1):
  Merge branch 'master' of git://blackhole.kfki.hu/nf

Vishwanath Pai (1):
  netfilter: ipset: Null pointer exception in ipset list:set

 include/uapi/linux/netfilter/nf_conntrack_common.h |  4 ++
 include/uapi/linux/netfilter/nfnetlink_queue.h |  2 +-
 net/netfilter/ipset/ip_set_hash_gen.h  |  2 +-
 net/netfilter/ipset/ip_set_list_set.c  |  9 +++--
 net/netfilter/nf_conntrack_helper.c| 39 +---
 net/netfilter/nf_conntrack_netlink.c   | 43 +++---
 net/netfilter/nfnetlink.c  |  2 +-
 net/netfilter/xt_hashlimit.c   | 25 +
 8 files changed, 86 insertions(+), 40 deletions(-)
--
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

[PATCH 8/8] netfilter: nfnetlink: remove static declaration from err_list

2017-02-23 Thread Pablo Neira Ayuso 
From: Liping Zhang 

Otherwise, different subsys will race to access the err_list, with holding
the different nfnl_lock(subsys_id).

But this will not happen now, since ->call_batch is only implemented by
nftables, so the err_list is protected by nfnl_lock(NFNL_SUBSYS_NFTABLES).

Signed-off-by: Liping Zhang 
Signed-off-by: Pablo Neira Ayuso 
---
 net/netfilter/nfnetlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index a09fa9fd8f3d..6fa448478cba 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -279,7 +279,7 @@ static void nfnetlink_rcv_batch(struct sk_buff *skb, struct 
nlmsghdr *nlh,
struct net *net = sock_net(skb->sk);
const struct nfnetlink_subsystem *ss;
const struct nfnl_callback *nc;
-   static LIST_HEAD(err_list);
+   LIST_HEAD(err_list);
u32 status;
int err;
 
-- 
2.1.4

--
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

[PATCH 5/8] netfilter: ipset: Null pointer exception in ipset list:set

2017-02-23 Thread Pablo Neira Ayuso 
From: Vishwanath Pai 

If we use before/after to add an element to an empty list it will cause
a kernel panic.

$> cat crash.restore
create a hash:ip
create b hash:ip
create test list:set timeout 5 size 4
add test b before a

$> ipset -R < crash.restore

Executing the above will crash the kernel.

Signed-off-by: Vishwanath Pai 
Reviewed-by: Josh Hunt 
Signed-off-by: Jozsef Kadlecsik 
---
 net/netfilter/ipset/ip_set_list_set.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/ipset/ip_set_list_set.c 
b/net/netfilter/ipset/ip_set_list_set.c
index 51077c53d76b..178d4eba013b 100644
--- a/net/netfilter/ipset/ip_set_list_set.c
+++ b/net/netfilter/ipset/ip_set_list_set.c
@@ -260,11 +260,14 @@ list_set_uadd(struct ip_set *set, void *value, const 
struct ip_set_ext *ext,
else
prev = e;
}
+
+   /* If before/after is used on an empty set */
+   if ((d->before > 0 && !next) ||
+   (d->before < 0 && !prev))
+   return -IPSET_ERR_REF_EXIST;
+
/* Re-add already existing element */
if (n) {
-   if ((d->before > 0 && !next) ||
-   (d->before < 0 && !prev))
-   return -IPSET_ERR_REF_EXIST;
if (!flag_exist)
return -IPSET_ERR_EXIST;
/* Update extensions */
-- 
2.1.4

--
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: netfilter: nft_ct: add zone id set support

2017-02-23 Thread Geert Uytterhoeven 
Hi Florian,

On Wed, Feb 22, 2017 at 8:02 PM, Linux Kernel Mailing List
 wrote:
> Web:
> https://git.kernel.org/torvalds/c/edee4f1e92458299505ff007733f676b00c516a1
> Commit: edee4f1e92458299505ff007733f676b00c516a1
> Parent: 5c178d81b69f08ca3195427a6ea9a46d9af23127
> Refname:refs/heads/master
> Author: Florian Westphal 
> AuthorDate: Fri Feb 3 13:35:50 2017 +0100
> Committer:  Pablo Neira Ayuso 
> CommitDate: Wed Feb 8 14:16:23 2017 +0100
>
> netfilter: nft_ct: add zone id set support
>
> zones allow tracking multiple connections sharing identical tuples,
> this is needed e.g. when tracking distinct vlans with overlapping ip
> addresses (conntrack is l2 agnostic).
>
> Thus the zone has to be set before the packet is picked up by the
> connection tracker.  This is done by means of 'conntrack templates' which
> are conntrack structures used solely to pass this info from one netfilter
> hook to the next.
>
> The iptables CT target instantiates these connection tracking templates
> once per rule, i.e. the template is fixed/tied to particular zone, can
> be read-only and therefore be re-used by as many skbs simultaneously as
> needed.
>
> We can't follow this model because we want to take the zone id from
> an sreg at rule eval time so we could e.g. fill in the zone id from
> the packets vlan id or a e.g. nftables key : value maps.
>
> To avoid cost of per packet alloc/free of the template, use a percpu
> template 'scratch' object and use the refcount to detect the (unlikely)
> case where the template is still attached to another skb (i.e., previous
> skb was nfqueued ...).
>
> Signed-off-by: Florian Westphal 
> Signed-off-by: Pablo Neira Ayuso 

> --- a/net/netfilter/nft_ct.c
> +++ b/net/netfilter/nft_ct.c

> @@ -407,6 +503,7 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
> unsigned int len;

With gcc-4.1.2 and -Os:

net/netfilter/nft_ct.c: In function ‘nft_ct_set_init’:
net/netfilter/nft_ct.c:503: warning: ‘len’ may be used
uninitialized in this function

> int err;
>
> +   priv->dir = IP_CT_DIR_MAX;
> priv->key = ntohl(nla_get_be32(tb[NFTA_CT_KEY]));
> switch (priv->key) {
>  #ifdef CONFIG_NF_CONNTRACK_MARK
> @@ -426,10 +523,28 @@ static int nft_ct_set_init(const struct nft_ctx *ctx,
> return err;
> break;
>  #endif
> +#ifdef CONFIG_NF_CONNTRACK_ZONES
> +   case NFT_CT_ZONE:
> +   if (!nft_ct_tmpl_alloc_pcpu())
> +   return -ENOMEM;
> +   nft_ct_pcpu_template_refcnt++;

Unlike for the other cases of the switch statement, "len" is not initialized
here...

> +   break;
> +#endif
> default:
> return -EOPNOTSUPP;
> }
>
> +   if (tb[NFTA_CT_DIRECTION]) {
> +   priv->dir = nla_get_u8(tb[NFTA_CT_DIRECTION]);
> +   switch (priv->dir) {
> +   case IP_CT_DIR_ORIGINAL:
> +   case IP_CT_DIR_REPLY:
> +   break;
> +   default:
> +   return -EINVAL;
> +   }
> +   }
> +
> priv->sreg = nft_parse_register(tb[NFTA_CT_SREG]);
> err = nft_validate_register_load(priv->sreg, len);

... and used here, which may lead to spurious failures of
nft_validate_register_load().

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
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