netdev - Re: [PATCH net-next] net: sched: act_tunnel

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: <vbf4l8ruckm.fsf@mellanox.com>
Date:   Mon, 25 Feb 2019 15:12:33 +0000
From:   Vlad Buslov <vladbu@...lanox.com>
To:     Davide Caratti <dcaratti@...hat.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jhs@...atatu.com" <jhs@...atatu.com>,
        "xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
        "jiri@...nulli.us" <jiri@...nulli.us>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "wenxu@...oud.cn" <wenxu@...oud.cn>, Roi Dayan <roid@...lanox.com>
Subject: Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata
 handling


On Mon 25 Feb 2019 at 14:04, Davide Caratti <dcaratti@...hat.com> wrote:
> On Mon, 2019-02-25 at 14:21 +0200, Vlad Buslov wrote:
>> Tunnel key action params->tcft_enc_metadata is only set when action is
>> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
>> dereferenced during tunnel key init and release without verifying that
>> action is if correct type, which causes NULL pointer dereference. Metadata
>> tunnel dst_cache is also leaked on action overwrite.
>> 
>> Fix metadata handling:
>> - Verify that metadata pointer is not NULL before dereferencing it in
>>   tunnel_key_init error handling code.
>
> hello Vlad,
>
> thanks a lot for fixing this!
>
> <...>
>  
>> @@ -384,10 +390,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>>  
>>  release_dst_cache:
>>  #ifdef CONFIG_DST_CACHE
>> -	dst_cache_destroy(&metadata->u.tun_info.dst_cache);
>> +	if (metadata)
>> +		dst_cache_destroy(&metadata->u.tun_info.dst_cache);
>>  #endif
>>  release_tun_meta:
>> -	dst_release(&metadata->dst);
>> +	if (metadata)
>> +		dst_release(&metadata->dst);
>
> on Linux 'net' tree we don't have commit 41411e2fd6b8 ("net/sched:
> act_tunnel_key: Add dst_cache support"), but still the above two lines can
> avoid a NULL dereference in tunnel_key_init() error path, in the following
> case:
>
> * create an action with tunnel "set", with success
> * replace the previous rule rule with tunnel "unset", and have a failure 
> here (e.g. allocation of 'params_new').
>
> At the cost of creating some conflicts during the merge, it would probably
> be safer to split this commit into two parts, one targeting 'net' and one
> targeting 'net-next', so that the first one can be proposed for stable
> backports (and also I can rebase/retest my 'goto chain' series on top of
> it :) )
>
> WDYT?

Makes sense. I'll send split patches.