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 <wu000...@umn.edu>
>>> Cc: Jay Vosburgh <j.vosbu...@gmail.com>
>>> Cc: Veaceslav Falico <vfal...@gmail.com>
>>> Cc: Andy Gospodarek <a...@greyhouse.net>
>>> Signed-off-by: Jamie Iles <ja...@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);
