[PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log

[PATCH nf-next 0/3] netfilter: fix some small bugs related to nft_log

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

This patchset is very small, aim to fix some bugs related to nftables log expr.

patch#1 fix a possible memory leak if the user specify the log prefix but the log
expr init fail.

patch#2 add a validity check of log level, otherwise user can specify an invalid
log level.

patch#3 fix "nft add rule filter input log snaplen 100" cannot work successfully,
the reason is similar to xt_NFLOG.

Liping Zhang (3):
  netfilter: nft_log: fix possible memory leak if log expr init fail
  netfilter: nft_log: check the validity of log level
  netfilter: nft_log: fix snaplen does not truncate packets

 net/netfilter/nft_log.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

-- 
2.5.5

[PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Suppose that we specify the NFTA_LOG_PREFIX, then NFTA_LOG_LEVEL
and NFTA_LOG_GROUP are specified together or nf_logger_find_get
call returns fail, i.e. expr init fail, memory leak will happen.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_log.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 713d668..e1b34ff 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -52,6 +52,14 @@ static int nft_log_init(const struct nft_ctx *ctx,
 	struct nft_log *priv = nft_expr_priv(expr);
 	struct nf_loginfo *li = &priv->loginfo;
 	const struct nlattr *nla;
+	int err;
+
+	li->type = NF_LOG_TYPE_LOG;
+	if (tb[NFTA_LOG_LEVEL] != NULL &&
+	    tb[NFTA_LOG_GROUP] != NULL)
+		return -EINVAL;
+	if (tb[NFTA_LOG_GROUP] != NULL)
+		li->type = NF_LOG_TYPE_ULOG;
 
 	nla = tb[NFTA_LOG_PREFIX];
 	if (nla != NULL) {
@@ -63,13 +71,6 @@ static int nft_log_init(const struct nft_ctx *ctx,
 		priv->prefix = (char *)nft_log_null_prefix;
 	}
 
-	li->type = NF_LOG_TYPE_LOG;
-	if (tb[NFTA_LOG_LEVEL] != NULL &&
-	    tb[NFTA_LOG_GROUP] != NULL)
-		return -EINVAL;
-	if (tb[NFTA_LOG_GROUP] != NULL)
-		li->type = NF_LOG_TYPE_ULOG;
-
 	switch (li->type) {
 	case NF_LOG_TYPE_LOG:
 		if (tb[NFTA_LOG_LEVEL] != NULL) {
@@ -96,7 +97,16 @@ static int nft_log_init(const struct nft_ctx *ctx,
 		break;
 	}
 
-	return nf_logger_find_get(ctx->afi->family, li->type);
+	err = nf_logger_find_get(ctx->afi->family, li->type);
+	if (err < 0)
+		goto err1;
+
+	return 0;
+
+err1:
+	if (priv->prefix != nft_log_null_prefix)
+		kfree(priv->prefix);
+	return err;
 }
 
 static void nft_log_destroy(const struct nft_ctx *ctx,
-- 
2.5.5

Re: [PATCH nf-next 1/3] netfilter: nft_log: fix possible memory leak if log expr init fail

[Pablo Neira Ayuso]

On Mon, Jul 18, 2016 at 08:44:15PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Suppose that we specify the NFTA_LOG_PREFIX, then NFTA_LOG_LEVEL
> and NFTA_LOG_GROUP are specified together or nf_logger_find_get
> call returns fail, i.e. expr init fail, memory leak will happen.

Applied, thanks.

[PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

User can specify the log level larger than 7(debug level) via
nfnetlink, this is invalid. So in this case, we should report
EINVAL to the userspace.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_log.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index e1b34ff..5f6f088 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -79,6 +79,11 @@ static int nft_log_init(const struct nft_ctx *ctx,
 		} else {
 			li->u.log.level = LOGLEVEL_WARNING;
 		}
+		if (li->u.log.level > LOGLEVEL_DEBUG) {
+			err = -EINVAL;
+			goto err1;
+		}
+
 		if (tb[NFTA_LOG_FLAGS] != NULL) {
 			li->u.log.logflags =
 				ntohl(nla_get_be32(tb[NFTA_LOG_FLAGS]));
-- 
2.5.5

Re: [PATCH nf-next 2/3] netfilter: nft_log: check the validity of log level

[Pablo Neira Ayuso]

On Mon, Jul 18, 2016 at 08:44:16PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> User can specify the log level larger than 7(debug level) via
> nfnetlink, this is invalid. So in this case, we should report
> EINVAL to the userspace.

Also applied, thanks.

[PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nft_log.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 5f6f088..24a73bb 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -92,6 +92,7 @@ static int nft_log_init(const struct nft_ctx *ctx,
 	case NF_LOG_TYPE_ULOG:
 		li->u.ulog.group = ntohs(nla_get_be16(tb[NFTA_LOG_GROUP]));
 		if (tb[NFTA_LOG_SNAPLEN] != NULL) {
+			li->u.ulog.flags |= NF_LOG_F_COPY_LEN;
 			li->u.ulog.copy_len =
 				ntohl(nla_get_be32(tb[NFTA_LOG_SNAPLEN]));
 		}
@@ -149,7 +150,7 @@ static int nft_log_dump(struct sk_buff *skb, const struct nft_expr *expr)
 		if (nla_put_be16(skb, NFTA_LOG_GROUP, htons(li->u.ulog.group)))
 			goto nla_put_failure;
 
-		if (li->u.ulog.copy_len) {
+		if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
 			if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
 					 htonl(li->u.ulog.copy_len)))
 				goto nla_put_failure;
-- 
2.5.5

Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Pablo Neira Ayuso]

On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.

Applied, thanks.

Will you send me a patch for nftables userspace to enable this flag?

It would be good to update the translation to make sure --nflog-size
map to snaplen and ignore --nflog-range.

Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Liping Zhang]

At 2016-07-20 02:16:00, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
>On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <liping.zhang@spreadtrum.com>
>> 
>> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
>> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
>> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
>
>Applied, thanks.
>
>Will you send me a patch for nftables userspace to enable this flag?
>
>It would be good to update the translation to make sure --nflog-size
>map to snaplen and ignore --nflog-range.

I find that nftables already support this feature, the following command mean to truncate packets
to 100 bytes before logging to the userspace:
  #nft add rule filter input log group 0 snaplen 100

Before my patch, it does not work.
And after apply my patch, it works as expected.

Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Pablo Neira Ayuso]

On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
> At 2016-07-20 02:16:00, "Pablo Neira Ayuso" <pablo@netfilter.org> wrote:
> >On Mon, Jul 18, 2016 at 08:44:17PM +0800, Liping Zhang wrote:
> >> From: Liping Zhang <liping.zhang@spreadtrum.com>
> >> 
> >> There's a similar problem in xt_NFLOG, and was fixed by commit 7643507fe8b5
> >> ("netfilter: xt_NFLOG: nflog-range does not truncate packets"). Only set
> >> copy_len here does not work, so we should enable NF_LOG_F_COPY_LEN also.
> >
> >Applied, thanks.
> >
> >Will you send me a patch for nftables userspace to enable this flag?
> >
> >It would be good to update the translation to make sure --nflog-size
> >map to snaplen and ignore --nflog-range.
> 
> I find that nftables already support this feature, the following command mean to truncate packets
> to 100 bytes before logging to the userspace:
>   #nft add rule filter input log group 0 snaplen 100
> 
> Before my patch, it does not work.
> And after apply my patch, it works as expected.

If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
see any reference to this flag being set.

Then, nft_log kernel has been actually working fine since the
beginning. It would be good anyway if we set this flag on from
userspace to leave things in consistent state.

Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Liping Zhang]

Hi Pablo,

2016-07-20 16:25 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
>> I find that nftables already support this feature, the following command mean to truncate packets
>> to 100 bytes before logging to the userspace:
>>   #nft add rule filter input log group 0 snaplen 100
>>
>> Before my patch, it does not work.
>> And after apply my patch, it works as expected.
>
> If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
> see any reference to this flag being set.
>

I use this NF_LOG_F_COPY_LEN flag as internal, and when the user specify
the NFTA_LOG_SNAPLEN attrribute, it will be enabled automatically.
See my codes:

               if (tb[NFTA_LOG_SNAPLEN] != NULL) {
+                       li->u.ulog.flags |= NF_LOG_F_COPY_LEN;

-               if (li->u.ulog.copy_len) {
+               if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
                         if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
                                         htonl(li->u.ulog.copy_len)))

So this flag will not be setted from userspace explicitly and will not
be dumped to the userspace.

> Then, nft_log kernel has been actually working fine since the
> beginning. It would be good anyway if we set this flag on from
> userspace to leave things in consistent state.

Do you mean this is something similar to "nflog-size" and
"nflog-range" in xt_NFLOG?
"nft log snaplen 100" cannot work since the beginning, so we should
keep it unchanged? And
maybe we should introduce a new option like "nft log snapsize 100"?

Re: [PATCH nf-next 3/3] netfilter: nft_log: fix snaplen does not truncate packets

[Pablo Neira Ayuso]

On Wed, Jul 20, 2016 at 05:00:45PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2016-07-20 16:25 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> > On Wed, Jul 20, 2016 at 07:00:13AM +0800, Liping Zhang wrote:
> >> I find that nftables already support this feature, the following command mean to truncate packets
> >> to 100 bytes before logging to the userspace:
> >>   #nft add rule filter input log group 0 snaplen 100
> >>
> >> Before my patch, it does not work.
> >> And after apply my patch, it works as expected.
> >
> > If I git grep NF_LOG_F_COPY_LEN from the nftables.git tree, I don't
> > see any reference to this flag being set.
> >
> 
> I use this NF_LOG_F_COPY_LEN flag as internal, and when the user specify
> the NFTA_LOG_SNAPLEN attrribute, it will be enabled automatically.
> See my codes:
> 
>                if (tb[NFTA_LOG_SNAPLEN] != NULL) {
> +                       li->u.ulog.flags |= NF_LOG_F_COPY_LEN;
> 
> -               if (li->u.ulog.copy_len) {
> +               if (li->u.ulog.flags & NF_LOG_F_COPY_LEN) {
>                          if (nla_put_be32(skb, NFTA_LOG_SNAPLEN,
>                                          htonl(li->u.ulog.copy_len)))
> 
> So this flag will not be setted from userspace explicitly and will not
> be dumped to the userspace.

That's fine the way it is then. Thanks for clarifying this.

> > Then, nft_log kernel has been actually working fine since the
> > beginning. It would be good anyway if we set this flag on from
> > userspace to leave things in consistent state.
> 
> Do you mean this is something similar to "nflog-size" and
> "nflog-range" in xt_NFLOG?
> "nft log snaplen 100" cannot work since the beginning, so we should
> keep it unchanged? And
> maybe we should introduce a new option like "nft log snapsize 100"?

In the particular case of nftables, given we're still below the 1.0
stage, we can just classify this as a bugfix, I wouldn't bother much
on this.

The only thing to care is to provide the right translation of
nflog-size to snaplen and you actually contributed this patch already,
so everything is fine :).