netdev - Re: [Patch net] tipc: call start and done ops directly in __tipc_nl_compat

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: <CAM_iQpU7YEoRy-y6xOv0Wt9t82e_NY1YGBQv1Yc9aQqhVPH=og@mail.gmail.com>
Date:   Tue, 4 Sep 2018 11:41:49 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Ying Xue <ying.xue@...driver.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        tipc-discussion@...ts.sourceforge.net,
        Jon Maloy <jon.maloy@...csson.com>
Subject: Re: [Patch net] tipc: call start and done ops directly in __tipc_nl_compat_dumpit()

On Tue, Sep 4, 2018 at 4:45 AM Ying Xue <ying.xue@...driver.com> wrote:
>
>
> On 09/04/2018 10:12 AM, Cong Wang wrote:
> > __tipc_nl_compat_dumpit() uses a netlink_callback on stack,
> > so the only way to align it with other ->dumpit() call path
> > is calling tipc_dump_start() and tipc_dump_done() directly
> > inside it. Otherwise ->dumpit() would always get NULL from
> > cb->args[0].
>
> Thank you for your fix Cong!
>
> Your solution is fine with me.
>
> When we align __tipc_nl_compat_dumpit() with ->dumpit() functions
> defined in tipc_genl_v2_ops[], cb->args[0] is used to save a
> rhashtable_iter object allocated in tipc_dump_start(), and the object
> will be retrieved with cb->args[0] in tipc_dump_done() and will be freed.
>
> But unfortunately cb->args[0] has been used to other purposes in
> tipc_nl_bearer_dump(), tipc_nl_node_dump_link(),
> tipc_nl_name_table_dump(), tipc_nl_node_dump() and tipc_nl_net_dump().
> It means cb->args[0] saved in __tipc_dump_start() will be overwritten in
> these ->dumpit() functions. As a consequence, not only the
> rhashtable_iter object allocated in tipc_dump_start() cannot be properly
> released in tipc_dump_done(), but also more kernel panics might be
> triggered in tipc_dump_done().

Ah, good catch!

The max utilization of cb->args is tipc_nl_name_table_dump():

net/tipc/name_table.c:  cb->args[0] = last_type;
net/tipc/name_table.c:  cb->args[1] = last_lower;
net/tipc/name_table.c:  cb->args[2] = last_key;
net/tipc/name_table.c:  cb->args[3] = done;

Looks like I should just use cb->args[4] for rhashtable iterator,
as we still have some room in cb->args[].