netdev - Re: [PATCH net] net: sched: fix refcount imbalance in actions

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: <55B9760B.10004@iogearbox.net>
Date:	Thu, 30 Jul 2015 02:55:39 +0200
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Cong Wang <cwang@...pensource.com>
CC:	David Miller <davem@...emloft.net>,
	Alexei Starovoitov <ast@...mgrid.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] net: sched: fix refcount imbalance in actions

On 07/30/2015 02:33 AM, Cong Wang wrote:
...
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index af427a3..bd63a39 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -53,8 +53,11 @@ int tcf_hash_release(struct tc_action *a, int bind)
>          if (p) {
>                  if (bind)
>                          p->tcfc_bindcnt--;
> -               else if (p->tcfc_bindcnt > 0)
> -                       return -EPERM;
> +               else {
> +                       if (p->tcfc_bindcnt > 0)
> +                               return -EPERM;
> +                       return ret;
> +               }

Hm, so this seems not correct: if we only ever increase tcfc_refcnt
when there's bind=1, and only ever decrease when bind=1, then we
will never free the action as we do start out from ref=1 in case
it has been added without initial binding, if I see this correctly.

>                  p->tcfc_refcnt--;
>                  if (p->tcfc_bindcnt <= 0 && p->tcfc_refcnt <= 0) {
> @@ -214,9 +217,10 @@ int tcf_hash_check(u32 index, struct tc_action
> *a, int bind)
>          struct tcf_hashinfo *hinfo = a->ops->hinfo;
>          struct tcf_common *p = NULL;
>          if (index && (p = tcf_hash_lookup(index, hinfo)) != NULL) {
> -               if (bind)
> +               if (bind) {
>                          p->tcfc_bindcnt++;
> -               p->tcfc_refcnt++;
> +                       p->tcfc_refcnt++;
> +               }
>                  a->priv = p;
>                  return 1;
>          }
> diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
> index a42a3b2..2685450 100644
> --- a/net/sched/act_mirred.c
> +++ b/net/sched/act_mirred.c
> @@ -98,6 +98,8 @@ static int tcf_mirred_init(struct net *net, struct
> nlattr *nla,
>                          return ret;
>                  ret = ACT_P_CREATED;
>          } else {
> +               if (bind)
> +                       return 0;
>                  if (!ovr) {
>                          tcf_hash_release(a, bind);
>                          return -EEXIST;
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html