Re: [PATCH for-rc] RDMA/srpt: Fix UAF when srpt_add_one() failed

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2024/8/2 10:28, Junxian Huang 写道:
> 
> 
> On 2024/8/2 5:55, Zhu Yanjun wrote:
>> 在 2024/8/1 15:44, Junxian Huang 写道:
>>> Currently cancel_work_sync() is not called when srpt_refresh_port()
>>> failed in srpt_add_one(). There is a probability that sdev has been
>>> freed while the previously initiated sport->work is still running,
>>> leading to a UAF as the log below:
>>>
>>> [  T880] ib_srpt MAD registration failed for hns_1-1.
>>> [  T880] ib_srpt srpt_add_one(hns_1) failed.
>>> [  T376] Unable to handle kernel paging request at virtual address 0000000000010008
>>> ...
>>> [  T376] Workqueue: events srpt_refresh_port_work [ib_srpt]
>>> ...
>>> [  T376] Call trace:
>>> [  T376]  srpt_refresh_port+0x94/0x264 [ib_srpt]
>>> [  T376]  srpt_refresh_port_work+0x1c/0x2c [ib_srpt]
>>> [  T376]  process_one_work+0x1d8/0x4cc
>>> [  T376]  worker_thread+0x158/0x410
>>> [  T376]  kthread+0x108/0x13c
>>> [  T376]  ret_from_fork+0x10/0x18
>>>
>>> Add cancel_work_sync() to the exception branch to fix this UAF.
>>
>> Can you share the method to reproduce this problem?
>> I am interested in this problem.
>>
>> Thanks,
>> Zhu Yanjun
>>
> 
> I was testing bonding in 5.10 kernel, doing
> 	ifenslave bond0 eth0 eth1; ifenslave -d bond0 eth0 eth1
> and
> 	ethtool --reset eth0 dedicated; ethtool --reset eth1 dedicated
> concurrently and infinitely.
> 
> But I think this problem has been fixed in latest mainline probably.
> Please look into my reply to Bart in v2.

Thanks a lot. I am working on srq. Because ib_srpt also supports srq, I 
just want to confirm if srq of ib_srpt can also work well on RoCEv2 and 
IB with this commit.

Zhu Yanjun

> 
> Junxian
> 
>>>
>>> Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
>>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com>
>>> ---
>>>    drivers/infiniband/ulp/srpt/ib_srpt.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> index 9632afbd727b..244e5c115bf7 100644
>>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>>> @@ -3148,8 +3148,8 @@ static int srpt_add_one(struct ib_device *device)
>>>    {
>>>        struct srpt_device *sdev;
>>>        struct srpt_port *sport;
>>> +    u32 i, j;
>>>        int ret;
>>> -    u32 i;
>>>          pr_debug("device = %p\n", device);
>>>    @@ -3226,7 +3226,6 @@ static int srpt_add_one(struct ib_device *device)
>>>            if (ret) {
>>>                pr_err("MAD registration failed for %s-%d.\n",
>>>                       dev_name(&sdev->device->dev), i);
>>> -            i--;
>>>                goto err_port;
>>>            }
>>>        }
>>> @@ -3241,6 +3240,8 @@ static int srpt_add_one(struct ib_device *device)
>>>        return 0;
>>>      err_port:
>>> +    for (j = i, i--; j > 0; j--)
>>> +        cancel_work_sync(&sdev->port[j - 1].work);
>>>        srpt_unregister_mad_agent(sdev, i);
>>>    err_cm:
>>>        if (sdev->cm_id)
>>