Re: [dpdk-dev] [PATCH v2] ring: fix use after free in ring release

[Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>]

01/05/2023 20:06, Honnappa Nagarahalli пишет:
> 
> 
>> -----Original Message-----
>> From: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>> Sent: Monday, May 1, 2023 7:32 AM
>> To: wangyunjian@huawei.com
>> Cc: dev@dpdk.org; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; konstantin.v.ananyev@yandex.ru;
>> luyicai@huawei.com; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH v2] ring: fix use after free in ring release
>>
>>
>>
>>> After the memzone is freed, it is not removed from the 'rte_ring_tailq'.
>>> If rte_ring_lookup is called at this time, it will cause a
>>> use-after-free problem. This change prevents that from happening.
>>>
>>> Fixes: 4e32101f9b01 ("ring: support freeing")
>>> Cc: stable@dpdk.org
>>>
>>> Suggested-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
>>> ---
>>> v2: update code suggested by Honnappa Nagarahalli
>>> ---
>>>   lib/ring/rte_ring.c | 8 +++-----
>>>   1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/lib/ring/rte_ring.c b/lib/ring/rte_ring.c index
>>> 8ed455043d..2755323b8a 100644
>>> --- a/lib/ring/rte_ring.c
>>> +++ b/lib/ring/rte_ring.c
>>> @@ -333,11 +333,6 @@ rte_ring_free(struct rte_ring *r)
>>>   		return;
>>>   	}
>>>
>>> -	if (rte_memzone_free(r->memzone) != 0) {
>>> -		RTE_LOG(ERR, RING, "Cannot free memory\n");
>>> -		return;
>>> -	}
>>> -
>>>   	ring_list = RTE_TAILQ_CAST(rte_ring_tailq.head, rte_ring_list);
>>>   	rte_mcfg_tailq_write_lock();
>>>
>>> @@ -354,6 +349,9 @@ rte_ring_free(struct rte_ring *r)
>>>
>>>   	TAILQ_REMOVE(ring_list, te, next);
>>>
>>> +	if (rte_memzone_free(r->memzone) != 0)
>>> +		RTE_LOG(ERR, RING, "Cannot free memory\n");
>>> +
>>
>> I nit: I think it is a bit better to first release the lock and then free the
>> memzone.
> I think both of our suggestions are contradictory. Any reason why you want to free outside the locked region?


Don't know what you mean by 'both suggestions' here.
I think I gave only one - move memzone_free() after tailq_write_unlock().
To be more precise:
1) rte_mcfg_tailq_write_lock();
...
2) TAILQ_REMOVE(...);
3) rte_mcfg_tailq_write_unlock();
4) rte_memzone_free(r->memzone);

As I remember, memzones are protected by their own lock (mlock), so
we don't need to hold qlock to free a memzone, after ring was already
removed from the ring_list.

> 
> I thought, since it belongs to the ring being freed, it makes sense to free it while holding the lock to avoid any race conditions (though, I have not checked what those are).


As I understand, it is ok with current design to grab mlock while 
holding qlock.
So, there is nothing wrong with current patch,
I just think that in that case it is excessive, and can be safely avoided.

> 
>> Apart from that, LGTM.
>> Acked-by: Konstantin Ananyev <konstantin.v.ananyev@yandex.ru>
>>
>>>   	rte_mcfg_tailq_write_unlock();
>>>
>>>   	rte_free(te);
>>> --
>>> 2.33.0
>>
>