linux-kernel - Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACGkMEuCWE-oHQXJYfQZh2k5s8BNWRgXW4wZrHyo7Z2=k89EeA@mail.gmail.com>
Date:   Fri, 25 Feb 2022 10:35:52 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Michael S. Tsirkin" <mst@...hat.com>
Cc:     virtualization <virtualization@...ts.linux-foundation.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] virtio_ring: aovid reading flag from the descriptor ring

On Fri, Feb 25, 2022 at 1:55 AM Michael S. Tsirkin <mst@...hat.com> wrote:
>
> Typo in the subject btw.
>
> minor tweaks to commit log below
>
> On Mon, Nov 08, 2021 at 04:13:24PM +0800, Jason Wang wrote:
> > Commit 72b5e8958738 ("virtio-ring: store DMA metadata in desc_extra
> > for split virtqueue") tries to make it possible for the driver to not
> > read from the descriptor ring to prevent the device from corrupting
> > the descriptor ring. But it still read
>
> reads
>
> >the descriptor flag from the
> > descriptor ring during buffer detach.
> >
> > This patch fixes
>
> fixes this
>
> >by always store
>
> storing
>

Will fix.

> >the descriptor flag no matter whether
> > DMA API is used and then we can avoid reading descriptor flag from the
> > descriptor ring. This eliminates the possibly of unexpected next
> > descriptor caused by the wrong flag (e.g the next flag).
> >
> > Signed-off-by: Jason Wang <jasowang@...hat.com>
>
>
> I'd also like the commit log to document what the issue is in a bit more depth.
> I think the main reason we are checking the dma API is this
>
>
> static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
>                                           unsigned int i)
> {
>         struct vring_desc_extra *extra = vq->split.desc_extra;
>         u16 flags;
>
>         if (!vq->use_dma_api)
>                 goto out;
>
>         ...
> }
>
>
> so I guess with a bad flag, what will happen is num_free will become too
> big is that right?

Yes, and it may have other implications and this is not easy to answer
since with the current code, next/flag is the only one that can be
controlled by the device. From what I've seen so far, it can cause an
early unmap for the descriptors in the free list. This may cause
several issues when using DMA API (e.g IOTLB for bouncing). Indeed
software IOMMU/IOTLB has done a lot of checks for this but we can't
solely depend on IOMMU/IOTLB to handle malicious inputs.

So my understanding is that, instead of trying to figuring out what
may happens if some flag or descriptor is modified by the malicious
device/hypervisor, we can simply eliminate all those possibilities
with minimal efforts and this is why we try not read anything from
shared memory area (e.g the descriptor ring etc) if possible.

>
>
>
>
> > ---
> >  drivers/virtio/virtio_ring.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 00f64f2f8b72..28734f4e57d3 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -583,7 +583,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> >       }
> >       /* Last one doesn't continue. */
> >       desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT);
> > -     if (!indirect && vq->use_dma_api)
> > +     if (!indirect)
> >               vq->split.desc_extra[prev & (vq->split.vring.num - 1)].flags &=
> >                       ~VRING_DESC_F_NEXT;
> >
>
> BTW I'm a bit confused why we need the & (vq->split.vring.num - 1) logic.
> Maybe it's time we stopped writing out descriptor then overwriting it -

Right since the next can be read from the descriptor ring directly (as
you said below, this needs to be fixed as well).

> e.g. return the desc_extra pointer from virtqueue_add_desc_split
> instead of an index. Worth checking what this does to performance.

Right, let me try to do that in the next version.

Thanks

>
>
> > @@ -713,7 +713,7 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> >       /* Put back on free list: unmap first-level descriptors and find end */
> >       i = head;
> >
> > -     while (vq->split.vring.desc[i].flags & nextflag) {
> > +     while (vq->split.desc_extra[i].flags & nextflag) {
> >               vring_unmap_one_split(vq, i);
> >               i = vq->split.desc_extra[i].next;
> >               vq->vq.num_free++;
> > --
> > 2.25.1
>