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: <4bde1d403d4ba9b51cf18bbaac1d46147011b959.camel@redhat.com>
Date:   Mon, 25 Feb 2019 15:04:27 +0100
From:   Davide Caratti <dcaratti@...hat.com>
To:     Vlad Buslov <vladbu@...lanox.com>, netdev@...r.kernel.org
Cc:     jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        davem@...emloft.net, wenxu@...oud.cn, roid@...lanox.com
Subject: Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata
 handling

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