netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vlad Buslov <vlad@buslov.dev>
To: Cong Wang <xiyou.wangcong@gmail.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	Vlad Buslov <vladbu@mellanox.com>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Jiri Pirko <jiri@resnulli.us>
Subject: Re: [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1()
Date: Fri, 25 Sep 2020 22:45:11 +0300	[thread overview]
Message-ID: <8736358wu0.fsf@buslov.dev> (raw)
In-Reply-To: <CAM_iQpXy4GuHidnLAL+euBaNaJGju6KFXBZ67WS_Pws58sD6+g@mail.gmail.com>

On Fri 25 Sep 2020 at 22:22, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Sep 25, 2020 at 8:24 AM Vlad Buslov <vlad@buslov.dev> wrote:
>> > +     if (TC_ACT_EXT_CMP(a->tcfa_action, TC_ACT_GOTO_CHAIN) &&
>> > +         !rcu_access_pointer(a->goto_chain)) {
>> > +             tcf_action_destroy_1(a, bind);
>> > +             NL_SET_ERR_MSG(extack, "can't use goto chain with NULL chain");
>> > +             return ERR_PTR(-EINVAL);
>> > +     }
>>
>> I don't think calling tcf_action_destoy_1() is enough here. Since you
>> moved this block before assigning cookie and releasing the module, you
>> also need to release them manually in addition to destroying the action
>> instance.
>>
>
> tcf_action_destoy_1() eventually calls free_tcf() which frees cookie and
> tcf_action_destroy() which releases module refcnt.
>
> What am I missing here?
>
> Thanks.

The memory referenced by the function local pointer "cookie" hasn't been
assigned yet to the a->act_cookie because in your patch you moved
goto_chain validation code before the cookie change. That means that if
user overwrites existing action, then action old a->act_cookie will be
freed by tcf_action_destroy_1() but new cookie that was allocated by
nla_memdup_cookie() will leak.

Regards,
Vlad


  reply	other threads:[~2020-09-25 20:35 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23  3:56 [Patch net 0/2] net_sched: fix a UAF in tcf_action_init() Cong Wang
2020-09-23  3:56 ` [Patch net 1/2] net_sched: defer tcf_idr_insert() in tcf_action_init_1() Cong Wang
2020-09-25 15:24   ` Vlad Buslov
2020-09-25 19:22     ` Cong Wang
2020-09-25 19:45       ` Vlad Buslov [this message]
2020-09-28 10:14         ` Davide Caratti
2020-09-28 17:38           ` Cong Wang
2020-09-23  3:56 ` [Patch net 2/2] net_sched: commit action insertions together Cong Wang
2020-09-25 15:51   ` Vlad Buslov
2020-09-25  2:47 ` [Patch net 0/2] net_sched: fix a UAF in tcf_action_init() David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8736358wu0.fsf@buslov.dev \
    --to=vlad@buslov.dev \
    --cc=jhs@mojatatu.com \
    --cc=jiri@resnulli.us \
    --cc=netdev@vger.kernel.org \
    --cc=vladbu@mellanox.com \
    --cc=xiyou.wangcong@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).