netdev - Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <eaeec889b3a07cea1347d48269e5964e@linux.dev>
Date:   Thu, 29 Jul 2021 03:19:01 +0000
From:   yajun.deng@...ux.dev
To:     "Pablo Neira Ayuso" <pablo@...filter.org>
Cc:     kadlec@...filter.org, fw@...len.de, roopa@...dia.com,
        nikolay@...dia.com, davem@...emloft.net, kuba@...nel.org,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        bridge@...ts.linux-foundation.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netfilter: nf_conntrack_bridge: Fix not free when
 error

July 29, 2021 12:18 AM, "Pablo Neira Ayuso" <pablo@...filter.org> wrote:

> On Mon, Jul 26, 2021 at 11:57:02AM +0800, Yajun Deng wrote:
> 
>> It should be added kfree_skb_list() when err is not equal to zero
>> in nf_br_ip_fragment().
>> 
>> Fixes: 3c171f496ef5 ("netfilter: bridge: add connection tracking system")
>> Signed-off-by: Yajun Deng <yajun.deng@...ux.dev>
>> ---
>> net/bridge/netfilter/nf_conntrack_bridge.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/bridge/netfilter/nf_conntrack_bridge.c
>> b/net/bridge/netfilter/nf_conntrack_bridge.c
>> index 8d033a75a766..059f53903eda 100644
>> --- a/net/bridge/netfilter/nf_conntrack_bridge.c
>> +++ b/net/bridge/netfilter/nf_conntrack_bridge.c
>> @@ -83,12 +83,16 @@ static int nf_br_ip_fragment(struct net *net, struct sock *sk,
>> 
>> skb->tstamp = tstamp;
>> err = output(net, sk, data, skb);
>> - if (err || !iter.frag)
>> - break;
>> -
>> + if (err) {
>> + kfree_skb_list(iter.frag);
>> + return err;
>> + }
>> +
>> + if (!iter.frag)
>> + return 0;
>> +
>> skb = ip_fraglist_next(&iter);
>> }
>> - return err;
> 
> Why removing this line above? It enters slow_path: on success.
> 
I used return rather than break, it wouldn't enter the slow_path.
> This patch instead will keep this aligned with IPv6.
> 
I think err and !iter.frag are not related, there is no need to put them in an if statement,
We still need to separate them after loop. So I separate them in loop and use return instead
of break. In addition, if you insist, I will accept your patch.
>> }
>> slow_path:
>> /* This is a linearized skbuff, the original geometry is lost for us.
>> --
>> 2.32.0