On Mon 25 Feb 2019 at 14:04, Davide Caratti <dcara...@redhat.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.
