[PATCH v2] nvmet: Avoid potential UAF in nvmet_req_complete()

[Damien Le Moal]

On 3/6/23 22:52, Sagi Grimberg wrote:
> Hey Damien,
> 
>> An nvme target ->queue_response() operation implementation may free the
>> request passed as argument. Such implementation potentially could result
>> in a use after free of the request pointer when percpu_ref_put() is
>> called in nvmet_req_complete().
> 
> Can you point me to which transport frees the request?

My prototype PCIe endpoint nvme function driver, not upstream yet.

The endpoint board keeps randomly crashing without this patch and not freeing
the request (embedded in a struct) in the ->queue_response() operation would
require *a lot* more code.

> 
>>
>> Avoid such problem by using a local variable to save the sq pointer
>> before calling __nvmet_req_complete(), thus avoiding dereferencing the
>> req pointer after that function call.
>>
>> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Damien Le Moal <damien.lemoal at opensource.wdc.com>
>> ---
>>
>> Changes from v1:
>>   * Added Fixes tag and cc stable
>>
>>   drivers/nvme/target/core.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index f66ed13d7c11..3935165048e7 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -756,8 +756,10 @@ static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
>>   
>>   void nvmet_req_complete(struct nvmet_req *req, u16 status)
>>   {
>> +	struct nvmet_sq *sq = req->sq;
>> +
>>   	__nvmet_req_complete(req, status);
>> -	percpu_ref_put(&req->sq->ref);
>> +	percpu_ref_put(&sq->ref);
>>   }
>>   EXPORT_SYMBOL_GPL(nvmet_req_complete);
>>   

-- 
Damien Le Moal
Western Digital Research