Re: [PATCH v3] vhost: avoid potential null pointer access

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/25/23 12:37, Morten Brørup wrote:
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Monday, 25 September 2023 10.15
>>
>> On 9/12/23 09:42, Li Feng wrote:
>>> If the user calls rte_vhost_vring_call() on a ring that has been
>>> invalidated, we will encounter SEGV.
>>>
>>> We should check the pointer firstly before accessing it.
>>>
>>> Signed-off-by: Li Feng <fengli@smartx.com>
>>> ---
>>> v2 -> v3:
>>> - Also fix the rte_vhost_vring_call_nonblock.
>>>
>>> v1 -> v2:
>>> - Fix rebase error.
>>>
>>>
>>>
>>>    lib/vhost/vhost.c | 14 ++++++++------
>>>    lib/vhost/vhost.h | 12 ++++++++++--
>>>    2 files changed, 18 insertions(+), 8 deletions(-)
>>
>>
>> Thanks for posting the fix, the segmentation fault may indeed happen
>> when injecting IRQ from the app directly using the Vhost API. It cannot
>> happen when vhost_vring_call() is calle directly from
>> rte_enqueue_burst/rte_dequeue_burst though.
>>
>> so I think below patch would be better:
>>
>> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
>> index eb6309b681..733e0ab289 100644
>> --- a/lib/vhost/vhost.c
>> +++ b/lib/vhost/vhost.c
>> @@ -1341,6 +1341,9 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx)
>>
>>           rte_rwlock_read_lock(&vq->access_lock);
>>
>> +       if (unlikely(!vq->access_ok))
>> +               return -1;
> 
> Don't you need to release the lock before returning here?

Of course yes, I actually caught it after sending my reply, it is 
already fixed locally.

But thanks for the review, much appreciated!
Maxime

>> +
>>           if (vq_is_packed(dev))
>>                   vhost_vring_call_packed(dev, vq);
>>           else
>> @@ -1371,6 +1374,9 @@ rte_vhost_vring_call_nonblock(int vid, uint16_t
>> vring_idx)
>>           if (rte_rwlock_read_trylock(&vq->access_lock))
>>                   return -EAGAIN;
>>
>> +       if (unlikely(!vq->access_ok))
>> +               return -1;
> 
> Don't you need to release the lock before returning here?
> 
>> +
>>           if (vq_is_packed(dev))
>>                   vhost_vring_call_packed(dev, vq);
>>           else
>>
>>
>> Do you confirm that fixes your issue?
>>
>> Thanks,
>> Maxime
>