[PATCH] netfilter: ctnetlink: move CTA_TIMEOUT case to outside

[Haishuang Yan]

When cda[CTA_TIMEOUT] is zero, ctnetlink_new_conntrack will
free allocated ct and return, so move it to outside to optimize
this situation.

Signed-off-by: Haishuang Yan 
---
 net/netfilter/nf_conntrack_netlink.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index a8be9b7..d1e6b1c 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1768,9 +1768,6 @@ static int change_seq_adj(struct nf_ct_seqadj *seq,
if (IS_ERR(ct))
return ERR_PTR(-ENOMEM);
 
-   if (!cda[CTA_TIMEOUT])
-   goto err1;
-
ct->timeout = nfct_time_stamp + ntohl(nla_get_be32(cda[CTA_TIMEOUT])) * 
HZ;
 
rcu_read_lock();
@@ -1944,7 +1941,7 @@ static int ctnetlink_new_conntrack(struct net *net, 
struct sock *ctnl,
if (nlh->nlmsg_flags & NLM_F_CREATE) {
enum ip_conntrack_events events;
 
-   if (!cda[CTA_TUPLE_ORIG] || !cda[CTA_TUPLE_REPLY])
+   if (!cda[CTA_TUPLE_ORIG] || !cda[CTA_TUPLE_REPLY] || 
!cda[CTA_TIMEOUT])
return -EINVAL;
if (otuple.dst.protonum != rtuple.dst.protonum)
return -EINVAL;
-- 
1.8.3.1



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

nfqueue accepted packet is disappeared

[Oleg]

  Hi all!

I have the test environment consists of 2 qemu VMs with next
network configuration:

VM2 eth0 --> [host br1] --> eth1 VM1 eth0 --> [host br0] --> Internet

I test nfqueue based filter running at VM1, which now simply accepts
all packets from eth1 immediately on callback entering:

static int
cb(struct nfq_q_handle *qh, struct nfgenmsg *nfmsg, struct nfq_data *nfad,
  void *data)
{
struct nfqnl_msg_packet_hdr *ph;
unsigned int verdict = NF_ACCEPT;
int ret;

ph = nfq_get_msg_packet_hdr(nfad);
ret = nfq_set_verdict(qh, ntohl(ph->packet_id), verdict, 0, NULL);
if (ret < 0)
ERR_OUT("nfq_set_verdict() error");

return ret;
}

ping from VM2 works well, but next command is delayed for several seconds:

~# telnet google.com 80

tcpdump on br0 and br1 shows that telnet sends 2 dns requests (A & )
and we see it on br1:

20:58:20.289941 IP 192.168.78.2.58758 > 8.8.8.8.53: 32688+ A? google.com. (28)
20:58:20.289985 IP 192.168.78.2.58758 > 8.8.8.8.53: 37919+ ? google.com. 
(28)
20:58:20.315317 IP 8.8.8.8.53 > 192.168.78.2.58758: 32688 6/0/0 A 
173.194.222.102, A 173.194.222.100, A 173.194.222.101, A 173.194.222.138, A 
173.194.222.113, A 173.194.222.139 (124)

but reply is only 1, because on br0 the second request is disappear:

20:58:20.290354 IP 192.168.77.32.58758 > 8.8.8.8.53: 32688+ A? google.com. (28)
20:58:20.314921 IP 8.8.8.8.53 > 192.168.77.32.58758: 32688 6/0/0 A 
173.194.222.102, A 173.194.222.100, A 173.194.222.101, A 173.194.222.138, A 
173.194.222.113, A 173.194.222.139 (124)

After 5 seconds telnet repeats 2 dns requests again and now it gets 2
replies. On br1:

20:58:25.294296 IP 192.168.78.2.58758 > 8.8.8.8.53: 32688+ A? google.com. (28)
20:58:25.299535 IP 8.8.8.8.53 > 192.168.78.2.58758: 32688 6/0/0 A 
173.194.222.102, A 173.194.222.100, A 173.194.222.101, A 173.194.222.138, A 
173.194.222.113, A 173.194.222.139 (124)
20:58:25.300172 IP 192.168.78.2.58758 > 8.8.8.8.53: 37919+ ? google.com. 
(28)
20:58:25.322761 IP 8.8.8.8.53 > 192.168.78.2.58758: 37919 1/0/0  
2a00:1450:4010:c0b::8a (56)

On br0 we now see 2 requests as expected:

20:58:25.295072 IP 192.168.77.32.58758 > 8.8.8.8.53: 32688+ A? google.com. (28)
20:58:25.299056 IP 8.8.8.8.53 > 192.168.77.32.58758: 32688 6/0/0 A 
173.194.222.102, A 173.194.222.100, A 173.194.222.101, A 173.194.222.138, A 
173.194.222.113, A 173.194.222.139 (124)
20:58:25.301021 IP 192.168.77.32.58758 > 8.8.8.8.53: 37919+ ? google.com. 
(28)
20:58:25.322186 IP 8.8.8.8.53 > 192.168.77.32.58758: 37919 1/0/0  
2a00:1450:4010:c0b::8a (56)

When i remove from iptables in VM1 nfqueue rule, telnet works well and
all packets are forwared.

So, my question is, what is happen with first  request and how
i can fix this?

kernel: 4.4.6
iptables: 1.4.21
libnetfilter_queue: 1.0.2

Thanks!


-- 
Олег Неманов (Oleg Nemanov)
--
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 1/3] scanner: add files in include dirs in alphabetical order.

[Arturo Borrero Gonzalez]

On 8 June 2017 at 12:17, Pablo Neira Ayuso  wrote:
> On Wed, Jun 07, 2017 at 09:40:53PM +0200, Arturo Borrero Gonzalez wrote:
>> On 7 June 2017 at 10:35, Ismo Puustinen  wrote:
>> >
>> > +static int directoryfilter(const struct dirent *de)
>> > +{
>> > +   if (strcmp(de->d_name, ".") == 0 ||
>> > +   strcmp(de->d_name, "..") == 0)
>> > +   return 0;
>> > +
>> > +   /* Accept other filenames. If we want to enable filtering based on
>> > +* filename suffix (*.nft), this would be the place to do it.
>> > +*/
>> > +
>>
>> This filter by suffix is good to have IMHO.
>> I guess that forcing users to explicitly create a file for nftables
>> (or at least give a specific suffix) reduces chances for user errors.
>
> You mean, this new include directory feature just takes *.nft files,
> right?
>

Yes,

> Then, to keep it consistent, we should also display a warning in
> include file with no .nft postfix. At deprecate the existing behaviour
> at some point, ie. bail out if you include a file that has no trailing
> .nft in its name.
>
> If we follow this path, all ruleset file will end up using .nft as
> a trailer in the name.
>

but perhaps it makes sense to differentiate two cases:
 * include a single file: accept arbitrary names
 * include a whole dir: accept only files ending in .nft

This seems to be what sysctl(8) does when loading a single file vs a directory.
I'm thinking in a case where you have a README in the directory or
other unrelated file.

If the idea is to allow drop files (a good idea indeed), then being
explicit is a good approach.

> Is there any other similar software following this approach? How is
> 'ferm' doing this?

ferm seems to load arbitrary files. In the docs they suggest using
.ferm files but the code
seems to allow whatever.
However, they have a set of regexp hardcoded to avoid loading things
like backups file an the like.
So, yes, probably forcing to .nft is sensible.
--
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] decnet: dn_rtmsg: Improve input length sanitization in dnrmg_receive_user_skb

[David Miller]

From: Mateusz Jurczyk 
Date: Wed, 7 Jun 2017 16:41:57 +0200

> On Wed, Jun 7, 2017 at 4:18 PM, Florian Westphal  wrote:
>> Mateusz Jurczyk  wrote:
>>> Verify that the length of the socket buffer is sufficient to cover the
>>> nlmsghdr structure before accessing the nlh->nlmsg_len field for further
>>> input sanitization. If the client only supplies 1-3 bytes of data in
>>> sk_buff, then nlh->nlmsg_len remains partially uninitialized and
>>> contains leftover memory from the corresponding kernel allocation.
>>> Operating on such data may result in indeterminate evaluation of the
>>> nlmsg_len < sizeof(*nlh) expression.
>>>
>>> The bug was discovered by a runtime instrumentation designed to detect
>>> use of uninitialized memory in the kernel. The patch prevents this and
>>> other similar tools (e.g. KMSAN) from flagging this behavior in the future.
>>
>> Instead of changing all the internal users wouldn't it be better
>> to add this check once in netlink_unicast_kernel?
>>
> 
> Perhaps. I must admit I'm not very familiar with this code
> area/interface, so I preferred to fix the few specific cases instead
> of submitting a general patch, which might have some unexpected side
> effects, e.g. behavior different from one of the internal clients etc.
> 
> If you think one check in netlink_unicast_kernel is a better way to do
> it, I'm happy to implement it like that.

Until we decide to add the check to netlink_unicast_kernel(), I'm applying
this and queueing it up for -stable.

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: [PATCH 1/3] scanner: add files in include dirs in alphabetical order.

[Pablo Neira Ayuso]

On Wed, Jun 07, 2017 at 09:40:53PM +0200, Arturo Borrero Gonzalez wrote:
> On 7 June 2017 at 10:35, Ismo Puustinen  wrote:
> >
> > +static int directoryfilter(const struct dirent *de)
> > +{
> > +   if (strcmp(de->d_name, ".") == 0 ||
> > +   strcmp(de->d_name, "..") == 0)
> > +   return 0;
> > +
> > +   /* Accept other filenames. If we want to enable filtering based on
> > +* filename suffix (*.nft), this would be the place to do it.
> > +*/
> > +
> 
> This filter by suffix is good to have IMHO.
> I guess that forcing users to explicitly create a file for nftables
> (or at least give a specific suffix) reduces chances for user errors.

You mean, this new include directory feature just takes *.nft files,
right?

Then, to keep it consistent, we should also display a warning in
include file with no .nft postfix. At deprecate the existing behaviour
at some point, ie. bail out if you include a file that has no trailing
.nft in its name.

If we follow this path, all ruleset file will end up using .nft as
a trailer in the name.

Is there any other similar software following this approach? How is
'ferm' doing this?
--
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