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

[Eric Dumazet <eric.dumazet@gmail.com>]

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@umn.edu>
>>> Cc: Jay Vosburgh <j.vosburgh@gmail.com>
>>> Cc: Veaceslav Falico <vfalico@gmail.com>
>>> Cc: Andy Gospodarek <andy@greyhouse.net>
>>> Signed-off-by: Jamie Iles <jamie@nuviainc.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);