Virtualization - Jun 2022 - [External] Re: [PATCH] virtio_ring : fix vring_packed_desc memory out of bounds bug

On Sun, Jun 12, 2022 at 07:02:25PM +0800, ?? wrote:
> Michael S. Tsirkin <mst at redhat.com> ?2022?6?11??? 08:35???
> >
> > On Sat, Jun 11, 2022 at 12:38:10AM +0800, ?? wrote:
> > > > This pattern was always iffy, but I don't think the
patch
> > > > improves the situation much. last_used_idx and
vq->packed.used_wrap_counter
> > > > can still get out of sync.
> > >
> > > Yes? You are absolutely correct, thanks for pointing out this
issue, I
> > > didn't take that into consideration,
> > > how about disabling interrupts before this code below:
> > >
> > > > vq->last_used_idx += vq->packed.desc_state[id].num;
> > > > if (unlikely(vq->last_used_idx >=
vq->packed.vring.num)) {
> > > >          vq->last_used_idx -= vq->packed.vring.num;
> > > >          vq->packed.used_wrap_counter ^= 1;
> > > > }
> > >
> > > it seems to be fine to just turn off the interrupts of the
current vring.
> > >
> > > BR
> >
> > That would make datapath significantly slower.
> >
> > >
> > > Michael S. Tsirkin <mst at redhat.com> ?2022?6?10???
22:50???
> > > >
> > > > On Fri, Jun 10, 2022 at 06:33:14PM +0800, huangjie.albert
wrote:
> > > > > ksoftirqd may consume the packet and it will call:
> > > > > virtnet_poll
> > > > >       -->virtnet_receive
> > > > >               -->virtqueue_get_buf_ctx
> > > > >                      
-->virtqueue_get_buf_ctx_packed
> > > > > and in virtqueue_get_buf_ctx_packed:
> > > > >
> > > > > vq->last_used_idx +=
vq->packed.desc_state[id].num;
> > > > > if (unlikely(vq->last_used_idx >=
vq->packed.vring.num)) {
> > > > >          vq->last_used_idx -=
vq->packed.vring.num;
> > > > >          vq->packed.used_wrap_counter ^= 1;
> > > > > }
> > > > >
> > > > > if at the same time, there comes a vring interrupt?in
vring_interrupt:
> > > > > we will call:
> > > > > vring_interrupt
> > > > >       -->more_used
> > > > >               -->more_used_packed
> > > > >                       -->is_used_desc_packed
> > > > > in is_used_desc_packed, the last_used_idx maybe >=
vq->packed.vring.num.
> > > > > so this could case a memory out of bounds bug.
> > > > >
> > > > > this patch is to fix this.
> > > > >
> > > > > Signed-off-by: huangjie.albert <huangjie.albert at
bytedance.com>
> > > >
> > > >
> > > > This pattern was always iffy, but I don't think the
patch
> > > > improves the situation much. last_used_idx and
vq->packed.used_wrap_counter
> > > > can still get out of sync.
> > > >
> > > > Maybe refactor code to keep everything in
vq->last_used_idx?
> > > >
> > > > Jason what is your take?
> > > >
> > > >
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c
b/drivers/virtio/virtio_ring.c
> > > > > index 13a7348cedff..d2abbb3a8187 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -1397,6 +1397,9 @@ static inline bool
is_used_desc_packed(const struct vring_virtqueue *vq,
> > > > >       bool avail, used;
> > > > >       u16 flags;
> > > > >
> > > > > +     if (idx >= vq->packed.vring.num)
> > > > > +             return false;
> > > > > +
> > > > >       flags =
le16_to_cpu(vq->packed.vring.desc[idx].flags);
> > > > >       avail = !!(flags & (1 <<
VRING_PACKED_DESC_F_AVAIL));
> > > > >       used = !!(flags & (1 <<
VRING_PACKED_DESC_F_USED));
> > > > > --
> > > > > 2.27.0
> > > >
> >
> 
> Michael S , thanks for your correction, there may be another simple
> solution here:
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 13a7348cedff..4db4db19f94a 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1397,6 +1397,9 @@ static inline bool is_used_desc_packed(const
> struct vring_virtqueue *vq,
>         bool avail, used;
>         u16 flags;
> 
> +       if (idx >= vq->packed.vring.num)
> +               return false;
> +
>         flags = le16_to_cpu(vq->packed.vring.desc[idx].flags);
>         avail = !!(flags & (1 << VRING_PACKED_DESC_F_AVAIL));
>         used = !!(flags & (1 << VRING_PACKED_DESC_F_USED));
> @@ -1453,8 +1456,9 @@ static void *virtqueue_get_buf_ctx_packed(struct
> virtqueue *_vq,
> 
>         vq->last_used_idx += vq->packed.desc_state[id].num;
>         if (unlikely(vq->last_used_idx >= vq->packed.vring.num)) {
> -               vq->last_used_idx -= vq->packed.vring.num;
>                 vq->packed.used_wrap_counter ^= 1;
> +               barrier();
> +               vq->last_used_idx -= vq->packed.vring.num;
>         }
> 
>         /*
> 
> vq->packed.used_wrap_counter  and  vq->last_used_idx only increased
> by the virtqueue_get_buf_ctx_packed, and
> so we can add a memory barrier and Changing the order in which
> last_used_idx  and used_wrap_counter  are assigned
> should temporarily solve the problem. But as you said, the code may
> need to be refactored to fully address these kinds of issues.
> 
> BR
this might solve the OOB access but not the problem that interrupt
might use an incorrect value to check for the used index.