netdev - Re: [RESEND PATCH] bonding: wait for sysfs kobject destruction before freeing struct slave

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]

Date:   Fri, 6 Nov 2020 17:17:12 +0100
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Jamie Iles <jamie@...iainc.com>,
        Eric Dumazet <eric.dumazet@...il.com>
Cc:     netdev@...r.kernel.org, Qiushi Wu <wu000273@....edu>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [RESEND PATCH] bonding: wait for sysfs kobject destruction before
 freeing struct slave



On 11/5/20 7:11 PM, Jamie Iles wrote:
> Hi Eric,
> 
> On Thu, Nov 05, 2020 at 01:49:03PM +0100, Eric Dumazet wrote:
>> On 11/5/20 9:41 AM, Jamie Iles wrote:
>>> syzkaller found that with CONFIG_DEBUG_KOBJECT_RELEASE=y, releasing a
>>> struct slave device could result in the following splat:
>>>
>>>
>>
>>> This is a potential use-after-free if the sysfs nodes are being accessed
>>> whilst removing the struct slave, so wait for the object destruction to
>>> complete before freeing the struct slave itself.
>>>
>>> Fixes: 07699f9a7c8d ("bonding: add sysfs /slave dir for bond slave devices.")
>>> Fixes: a068aab42258 ("bonding: Fix reference count leak in bond_sysfs_slave_add.")
>>> Cc: Qiushi Wu <wu000273@....edu>
>>> Cc: Jay Vosburgh <j.vosburgh@...il.com>
>>> Cc: Veaceslav Falico <vfalico@...il.com>
>>> Cc: Andy Gospodarek <andy@...yhouse.net>
>>> Signed-off-by: Jamie Iles <jamie@...iainc.com>
>>> ---
> ...
>> This seems weird, are we going to wait for a completion while RTNL is held ?
>> I am pretty sure this could be exploited by malicious user/syzbot.
>>
>> The .release() handler could instead perform a refcounted
>> bond_free_slave() action.
> 
> Okay, so were you thinking along the lines of this moving the lifetime 
> of the slave to the kobject?
> 

This seems a bit too complex for a bug fix.

Instead of adding a completion, you could add a refcount_t, and
make bond_free_slave() a wrapper like

static inline void bond_free_slave(struct slave *slave)
{
   if (refcount_dec_and_test(&slave->refcnt))
       __bond_free_slave(slave);
}

Once the kobj is successfully activated, you would
need a refcount_inc(&slave->refcount);

Total patch would be smaller and easier to review.

The kobj .release handler would simply call bond_free_slave(slave);