Re: [PATCHv2 1/1] RDMA/rxe: Fix BUG: KASAN: null-ptr-deref in rxe_qp_do_cleanup

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

在 2022/7/5 18:35, Haris Iqbal 写道:
> On Tue, Jul 5, 2022 at 8:28 AM <yanjun.zhu@linux.dev> wrote:
>>
>> From: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> The function rxe_create_qp calls rxe_qp_from_init. If some error
>> occurs, the error handler of function rxe_qp_from_init will set
>> both scq and rcq to NULL.
>>
>> Then rxe_create_qp calls rxe_put to handle qp. In the end,
>> rxe_qp_do_cleanup is called by rxe_put. rxe_qp_do_cleanup directly
>> accesses scq and rcq before checking them. This will cause
>> null-ptr-deref error.
>>
>> The call graph is as below:
>>
>> rxe_create_qp {
>>    ...
>>    rxe_qp_from_init {
>>      ...
>>    err1:
>>      ...
>>      qp->rcq = NULL;  <---rcq is set to NULL
>>      qp->scq = NULL;  <---scq is set to NULL
>>      ...
>>    }
>>
>> qp_init:
>>    rxe_put{
>>      ...
>>      rxe_qp_do_cleanup {
>>        ...
>>        atomic_dec(&qp->scq->num_wq); <--- scq is accessed
>>        ...
>>        atomic_dec(&qp->rcq->num_wq); <--- rcq is accessed
>>      }
>> }
>>
>> Fixes: 4703b4f0d94a ("RDMA/rxe: Enforce IBA C11-17")
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>> ---
>> V1->V2: Describe the error flows.
>> ---
>>   drivers/infiniband/sw/rxe/rxe_qp.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
>> index 22e9b85344c3..b79e1b43454e 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_qp.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_qp.c
>> @@ -804,13 +804,15 @@ static void rxe_qp_do_cleanup(struct work_struct *work)
>>          if (qp->rq.queue)
>>                  rxe_queue_cleanup(qp->rq.queue);
>>
>> -       atomic_dec(&qp->scq->num_wq);
>> -       if (qp->scq)
>> +       if (qp->scq) {
>> +               atomic_dec(&qp->scq->num_wq);
>>                  rxe_put(qp->scq);
>> +       }
>>
>> -       atomic_dec(&qp->rcq->num_wq);
>> -       if (qp->rcq)
>> +       if (qp->rcq) {
>> +               atomic_dec(&qp->rcq->num_wq);
>>                  rxe_put(qp->rcq);
>> +       }
>>
>>          if (qp->pd)
>>                  rxe_put(qp->pd);
>> --
>> 2.34.1
> 
> Looks good.
> 
> Reviewed-by: Md Haris Iqbal <haris.iqbal@ionos.com>
> 
> Should the check for "rxe_cleanup_task(&qp->comp.task);" also happen
> in this commit? or would it be covered in a different one?

It is in a different commit. I will send it out very soon.

Zhu Yanjun

> 
>>