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

["Morten Brørup" <mb@smartsharesystems.com>]

> 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?

> +
>          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