[Zesty][PATCH v2 0/2] net sched actions: allocate act cookie early

[Fabian Grünbichler]

that should have been [PATCH 2/2] in the subject, sorry for the
confusion.

On Mon, Apr 24, 2017 at 04:26:39PM +0200, Fabian Grünbichler wrote:
> From: Wolfgang Bumiller <w.bumiller at proxmox.com>
> 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1682368
> 
> Policing filters do not use the TCA_ACT_* enum and the tb[]
> nlattr array in tcf_action_init_1() doesn't get filled for
> them so we should not try to look for a TCA_ACT_COOKIE
> attribute in the then uninitialized array.
> The error handling in cookie allocation then calls
> tcf_hash_release() leading to invalid memory access later
> on.
> Additionally, if cookie allocation fails after an already
> existing non-policing filter has successfully been changed,
> tcf_action_release() should not be called, also we would
> have to roll back the changes in the error handling, so
> instead we now allocate the cookie early and assign it on
> success at the end.
> 
> CVE-2017-7979
> Fixes: 1045ba77a596 ("net sched actions: Add support for user cookies")
> Signed-off-by: Wolfgang Bumiller <w.bumiller at proxmox.com>
> Acked-by: Jamal Hadi Salim <jhs at mojatatu.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit e0535ce58b92d7baf0b33284a6c4f8f0338f943e)
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
>  net/sched/act_api.c | 55 +++++++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index e336f30..bdbc7a9 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -532,20 +532,20 @@ int tcf_action_dump(struct sk_buff *skb, struct list_head *actions,
>  	return err;
>  }
>  
> -static int nla_memdup_cookie(struct tc_action *a, struct nlattr **tb)
> +static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)
>  {
> -	a->act_cookie = kzalloc(sizeof(*a->act_cookie), GFP_KERNEL);
> -	if (!a->act_cookie)
> -		return -ENOMEM;
> +	struct tc_cookie *c = kzalloc(sizeof(*c), GFP_KERNEL);
> +	if (!c)
> +		return NULL;
>  
> -	a->act_cookie->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
> -	if (!a->act_cookie->data) {
> -		kfree(a->act_cookie);
> -		return -ENOMEM;
> +	c->data = nla_memdup(tb[TCA_ACT_COOKIE], GFP_KERNEL);
> +	if (!c->data) {
> +		kfree(c);
> +		return NULL;
>  	}
> -	a->act_cookie->len = nla_len(tb[TCA_ACT_COOKIE]);
> +	c->len = nla_len(tb[TCA_ACT_COOKIE]);
>  
> -	return 0;
> +	return c;
>  }
>  
>  struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> @@ -554,6 +554,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  {
>  	struct tc_action *a;
>  	struct tc_action_ops *a_o;
> +	struct tc_cookie *cookie = NULL;
>  	char act_name[IFNAMSIZ];
>  	struct nlattr *tb[TCA_ACT_MAX + 1];
>  	struct nlattr *kind;
> @@ -569,6 +570,18 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  			goto err_out;
>  		if (nla_strlcpy(act_name, kind, IFNAMSIZ) >= IFNAMSIZ)
>  			goto err_out;
> +		if (tb[TCA_ACT_COOKIE]) {
> +			int cklen = nla_len(tb[TCA_ACT_COOKIE]);
> +
> +			if (cklen > TC_COOKIE_MAX_SIZE)
> +				goto err_out;
> +
> +			cookie = nla_memdup_cookie(tb);
> +			if (!cookie) {
> +				err = -ENOMEM;
> +				goto err_out;
> +			}
> +		}
>  	} else {
>  		err = -EINVAL;
>  		if (strlcpy(act_name, name, IFNAMSIZ) >= IFNAMSIZ)
> @@ -607,20 +620,12 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  	if (err < 0)
>  		goto err_mod;
>  
> -	if (tb[TCA_ACT_COOKIE]) {
> -		int cklen = nla_len(tb[TCA_ACT_COOKIE]);
> -
> -		if (cklen > TC_COOKIE_MAX_SIZE) {
> -			err = -EINVAL;
> -			tcf_hash_release(a, bind);
> -			goto err_mod;
> -		}
> -
> -		if (nla_memdup_cookie(a, tb) < 0) {
> -			err = -ENOMEM;
> -			tcf_hash_release(a, bind);
> -			goto err_mod;
> +	if (name == NULL && tb[TCA_ACT_COOKIE]) {
> +		if (a->act_cookie) {
> +			kfree(a->act_cookie->data);
> +			kfree(a->act_cookie);
>  		}
> +		a->act_cookie = cookie;
>  	}
>  
>  	/* module count goes up only when brand new policy is created
> @@ -635,6 +640,10 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
>  err_mod:
>  	module_put(a_o->owner);
>  err_out:
> +	if (cookie) {
> +		kfree(cookie->data);
> +		kfree(cookie);
> +	}
>  	return ERR_PTR(err);
>  }
>  
> -- 
> 2.1.4
> 
> 
> 
> -- 
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team