Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata handling

[Davide Caratti <dcaratti@redhat.com>]

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