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
