Message-ID: <20230531062108-mutt-send-email-mst@kernel.org>
Date:   Wed, 31 May 2023 06:24:58 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Jason Wang <jasowang@...hat.com>
Cc:     xuanzhuo@...ux.alibaba.com,
        virtualization@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] virtio_ring: validate used buffer length

On Wed, May 31, 2023 at 04:26:38PM +0800, Jason Wang wrote:
> On Wed, May 31, 2023 at 3:36 PM Jason Wang <jasowang@...hat.com> wrote:
> >
> > On Wed, May 31, 2023 at 1:50 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > >
> > > On Wed, May 31, 2023 at 09:05:00AM +0800, Jason Wang wrote:
> > > > On Mon, May 29, 2023 at 6:03 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > >
> > > > > On Mon, May 29, 2023 at 09:18:10AM +0800, Jason Wang wrote:
> > > > > > On Sun, May 28, 2023 at 3:57 PM Michael S. Tsirkin <mst@...hat.com> wrote:
> > > > > > >
> > > > > > > On Fri, May 26, 2023 at 02:30:41PM +0800, Jason Wang wrote:
> > > > > > > > This patch validate
> > > > > > >
> > > > > > > validates
> > > > > > >
> > > > > > > > the used buffer length provided by the device
> > > > > > > > before trying to use it.
> > > > > > >
> > > > > > > before returning it to caller
> > > > > > >
> > > > > > > > This is done by remembering the in buffer
> > > > > > > > length in a dedicated array during virtqueue_add(), then we can fail
> > > > > > > > the virtqueue_get_buf() when we find the device is trying to give us a
> > > > > > > > used buffer length which is greater than we stored before.
> > > > > > >
> > > > > > > than what we stored
> > > > > > >
> > > > > > > >
> > > > > > > > This validation is disable
> > > > > > >
> > > > > > > disabled
> > > > > > >
> > > > > > > > by default via module parameter to unbreak
> > > > > > > > some existing devices since some legacy devices are known to report
> > > > > > > > buggy used length.
> > > > > > > >
> > > > > > > > Signed-off-by: Jason Wang <jasowang@...hat.com>
> > > > > > >
> > > > > > > First I'm not merging this without more data about
> > > > > > > what is known to be broken and what is known to work well
> > > > > > > in the commit log. And how exactly do things work if used length
> > > > > > > is wrong?
> > > > > >
> > > > > > Assuming the device is malicious, it would be very hard to answer.
> > > > > > Auditing and fuzzing won't cover every case. Instead of trying to seek
> > > > > > the answer, we can simply make sure the used in buffer length is
> > > > > > validated then we know we're fine or not.
> > > > >
> > > > > To restate the question, you said above "some legacy devices are known
> > > > > to report buggy used length". If they report buggy length then how
> > > > > can things work?
> > > >
> > > > The validation is disabled for legacy device (as stated in the changelog):
> > > >
> > > > static bool vring_needs_used_validation(const struct virtio_device *vdev)
> > > > {
> > > >         /*
> > > >          * Several legacy devices are known to produce buggy used
> > > >          * length. In order to let driver work, we won't validate used
> > > >          * buffer length in this case.
> > > >          */
> > > >         if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > > >                 return false;
> > > >         if (force_used_validation)
> > > >                 return true;
> > > >         return false;
> > > > }
> > > >
> > > > This seems to be what we've agreed in last version:
> > > >
> > > > https://lore.kernel.org/all/CANLsYkxfhamUU0bb4j7y6N4_G9odKxLCjXxgXEx4SJ6_Kf+M2Q@mail.gmail.com/T/#m31f3b06f9032beec175c312dfa2532cb08b15c56
> > > >
> > > > Thanks
> > > >
> > >
> > > I don't get it. You wrote:
> > >
> > >         This validation is disable
> > >         by default via module parameter to unbreak
> > >         some existing devices since some legacy devices are known to report
> > >         buggy used length.
> > >
> > > which devices?
> >
> > legacy rpmsg and vsock device (before 49d8c5ffad07) at least.
> >
> > > why do you need a module parameter?
> >
> > If we enable it unconditionally for modern devices, it may break some
> > buggy moden device (vsock without a fix as an example).
> >
> > >
> > >
> > > > >
> > > > > > > Second what's wrong with dma_desc_extra that we already maintain?
> > > > > > > Third motivation - it's part and parcel of the hardening effort yes?
> > > > > >
> > > > > > They are different. dma_desc_extra is for a descriptor ring, but this
> > > > > > is for a used ring. Technically we can go back to iterate on the
> > > > > > descriptor ring for a legal used in buffer length. But it will have
> > > > > > worse performance.
> > > > >
> > > > > I don't really understand. We already iterate when we unmap -
> > > > > all that is necessary is to subtract it from used length, if at
> > > > > the end of the process it is >0 then we know used length is too
> > > > > large.
> > > >
> > > > Yes, but it is the job that is done in the driver level not the virtio
> > > > core.
> > >
> > > What job?
> >
> > I meant the driver can do the validation since it has the knowledge of
> > the buffer length if it wants.
> >
> > > unmap is done in detach_buf_split and detach_buf_packed respectively.
> > > vring_desc_extra isn't even visible outside drivers/virtio/virtio_ring.c
> >
> > desc_extra doesn't contain buffer length for the case of indirect
> > descriptors. So we need to iterate in the descriptors when it looks
> > expensive if we don't need unmap.
> >
> > Thanks
> >
> > >
> > > For drivers that do unmap at driver level - I guess they can do
> > > validation there too.
> > >
> > > > Validation in virtio core is still necessary since they're
> > > > working at different levels and it's hard to force the validation in
> > > > all drivers by codes. Last version introduces a
> > > > suppress_driver_validation to allow the driver to suppress the core
> > > > validation which seems not good, we need a way to force the
> > > > virtio_ring code to do validation before.
> > >
> > > Why do we? If driver validates length virtio_ring does not need to
> > > validate.
> 
> To be more safe, there's no guarantee that there's no bug in the driver.

Extra options increase testing matrix size so - there be bugs.
We need to make these decisions for (most) users.

> >  If driver does not use length virtio_ring does not need to
> > > validate.
> 
> This could be done on top assuming the validation is disabled by
> default. But if the administrator wants to have belt and braces we
> need to leave an option for them.
> 
> Thanks

No, we don't regress then fix on top.
As for mod parameter I am not impressed -
no one's going to have the time or inclination to do the requisite
testing to know whether the module parameter is safe.

> > core can provide this service for the gazillion non
> > > performance critical drivers that just want to keep things simple,
> > > but the 4-5 critical ones can do their own validation if they want to.
> > >
> > > > Or such stuff could be added
> > > > on top since the validation is by default anyway.
> > > >
> > > > Thanks
> > >
> > >
> > >
> > > > >
> > > > >
> > > > > > > I'd like to know the fate of VIRTIO_HARDEN_NOTIFICATION before
> > > > > > > we do more hardening. If it's irrevocably broken let's rip it out?
> > > > > >
> > > > > > So the plan is
> > > > > >
> > > > > > 1) finish used ring validation (this had been proposed, merged and
> > > > > > reverted before notification hardening)
> > > > > > 2) do notification hardening on top.
> > > > > >
> > > > > > So let's leave it as is and I will do a rework after we finalize the
> > > > > > used ring validation.
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > > Changes since V4:
> > > > > > > > - drop the flat for driver to suppress the check
> > > > > > > > - validation is disabled by default
> > > > > > > > - don't do validation for legacy device
> > > > > > > > - rebase and support virtqueue resize
> > > > > > > > ---
> > > > > > > >  drivers/virtio/virtio_ring.c | 75 ++++++++++++++++++++++++++++++++++++
> > > > > > > >  1 file changed, 75 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > index 143f380baa1c..5b151605aaf8 100644
> > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > @@ -15,6 +15,9 @@
> > > > > > > >  #include <linux/spinlock.h>
> > > > > > > >  #include <xen/xen.h>
> > > > > > > >
> > > > > > > > +static bool force_used_validation = false;
> > > > > > > > +module_param(force_used_validation, bool, 0444);
> > > > > > > > +
> > > > > > > >  #ifdef DEBUG
> > > > > > > >  /* For development, we want to crash whenever the ring is screwed. */
> > > > > > > >  #define BAD_RING(_vq, fmt, args...)                          \
> > > > > > > > @@ -105,6 +108,9 @@ struct vring_virtqueue_split {
> > > > > > > >       struct vring_desc_state_split *desc_state;
> > > > > > > >       struct vring_desc_extra *desc_extra;
> > > > > > > >
> > > > > > > > +     /* Maximum in buffer length, NULL means no used validation */
> > > > > > > > +     u32 *buflen;
> > > > > > > > +
> > > > > > > >       /* DMA address and size information */
> > > > > > > >       dma_addr_t queue_dma_addr;
> > > > > > > >       size_t queue_size_in_bytes;
> > > > > > > > @@ -145,6 +151,9 @@ struct vring_virtqueue_packed {
> > > > > > > >       struct vring_desc_state_packed *desc_state;
> > > > > > > >       struct vring_desc_extra *desc_extra;
> > > > > > > >
> > > > > > > > +     /* Maximum in buffer length, NULL means no used validation */
> > > > > > > > +     u32 *buflen;
> > > > > > > > +
> > > > > > > >       /* DMA address and size information */
> > > > > > > >       dma_addr_t ring_dma_addr;
> > > > > > > >       dma_addr_t driver_event_dma_addr;
> > > > > > > > @@ -552,6 +561,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > > > >       unsigned int i, n, avail, descs_used, prev, err_idx;
> > > > > > > >       int head;
> > > > > > > >       bool indirect;
> > > > > > > > +     u32 buflen = 0;
> > > > > > > >
> > > > > > > >       START_USE(vq);
> > > > > > > >
> > > > > > > > @@ -635,6 +645,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > > > >                                                    VRING_DESC_F_NEXT |
> > > > > > > >                                                    VRING_DESC_F_WRITE,
> > > > > > > >                                                    indirect);
> > > > > > > > +                     buflen += sg->length;
> > > > > > > >               }
> > > > > > > >       }
> > > > > > > >       /* Last one doesn't continue. */
> > > > > > > > @@ -675,6 +686,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > > > >       else
> > > > > > > >               vq->split.desc_state[head].indir_desc = ctx;
> > > > > > > >
> > > > > > > > +     /* Store in buffer length if necessary */
> > > > > > > > +     if (vq->split.buflen)
> > > > > > > > +             vq->split.buflen[head] = buflen;
> > > > > > > > +
> > > > > > > >       /* Put entry in available array (but don't update avail->idx until they
> > > > > > > >        * do sync). */
> > > > > > > >       avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1);
> > > > > > > > @@ -861,6 +876,11 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > > > >               BAD_RING(vq, "id %u is not a head!\n", i);
> > > > > > > >               return NULL;
> > > > > > > >       }
> > > > > > > > +     if (vq->split.buflen && unlikely(*len > vq->split.buflen[i])) {
> > > > > > > > +             BAD_RING(vq, "used len %d is larger than max in buffer len %u\n",
> > > > > > > > +                     *len, vq->split.buflen[i]);
> > > > > > > > +             return NULL;
> > > > > > > > +     }
> > > > > > > >
> > > > > > > >       /* detach_buf_split clears data, so grab it now. */
> > > > > > > >       ret = vq->split.desc_state[i].data;
> > > > > > > > @@ -1085,10 +1105,25 @@ static void vring_free_split(struct vring_virtqueue_split *vring_split,
> > > > > > > >                        vring_split->queue_dma_addr,
> > > > > > > >                        dma_dev);
> > > > > > > >
> > > > > > > > +     kfree(vring_split->buflen);
> > > > > > > >       kfree(vring_split->desc_state);
> > > > > > > >       kfree(vring_split->desc_extra);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +static bool vring_needs_used_validation(const struct virtio_device *vdev)
> > > > > > > > +{
> > > > > > > > +     /*
> > > > > > > > +      * Several legacy devices are known to produce buggy used
> > > > > > > > +      * length. In order to let driver work, we won't validate used
> > > > > > > > +      * buffer length in this case.
> > > > > > > > +      */
> > > > > > > > +     if (!virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> > > > > > > > +             return false;
> > > > > > > > +     if (force_used_validation)
> > > > > > > > +             return true;
> > > > > > > > +     return false;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > >  static int vring_alloc_queue_split(struct vring_virtqueue_split *vring_split,
> > > > > > > >                                  struct virtio_device *vdev,
> > > > > > > >                                  u32 num,
> > > > > > > > @@ -1137,7 +1172,19 @@ static int vring_alloc_queue_split(struct vring_virtqueue_split *vring_split,
> > > > > > > >       vring_split->vring_align = vring_align;
> > > > > > > >       vring_split->may_reduce_num = may_reduce_num;
> > > > > > > >
> > > > > > > > +     if (vring_needs_used_validation(vdev)) {
> > > > > > > > +             vring_split->buflen =
> > > > > > > > +                     kmalloc_array(num, sizeof(*vring_split->buflen),
> > > > > > > > +                                   GFP_KERNEL);
> > > > > > > > +             if (!vring_split->buflen)
> > > > > > > > +                     goto err_buflen;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > >       return 0;
> > > > > > > > +
> > > > > > > > +err_buflen:
> > > > > > > > +     vring_free_split(vring_split, vdev, dma_dev);
> > > > > > > > +     return -ENOMEM;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static struct virtqueue *vring_create_virtqueue_split(
> > > > > > > > @@ -1297,6 +1344,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > > > > > >       unsigned int i, n, err_idx;
> > > > > > > >       u16 head, id;
> > > > > > > >       dma_addr_t addr;
> > > > > > > > +     u32 buflen = 0;
> > > > > > > >
> > > > > > > >       head = vq->packed.next_avail_idx;
> > > > > > > >       desc = alloc_indirect_packed(total_sg, gfp);
> > > > > > > > @@ -1325,6 +1373,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > > > > > >                       desc[i].addr = cpu_to_le64(addr);
> > > > > > > >                       desc[i].len = cpu_to_le32(sg->length);
> > > > > > > >                       i++;
> > > > > > > > +                     if (n >= out_sgs)
> > > > > > > > +                             buflen += sg->length;
> > > > > > > >               }
> > > > > > > >       }
> > > > > > > >
> > > > > > > > @@ -1379,6 +1429,10 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> > > > > > > >       vq->packed.desc_state[id].last = id;
> > > > > > > >       vq->packed.desc_state[id].premapped = premapped;
> > > > > > > >
> > > > > > > > +     /* Store in buffer length if necessary */
> > > > > > > > +     if (vq->packed.buflen)
> > > > > > > > +             vq->packed.buflen[id] = buflen;
> > > > > > > > +
> > > > > > > >       vq->num_added += 1;
> > > > > > > >
> > > > > > > >       pr_debug("Added buffer head %i to %p\n", head, vq);
> > > > > > > > @@ -1416,6 +1470,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > > > > >       __le16 head_flags, flags;
> > > > > > > >       u16 head, id, prev, curr, avail_used_flags;
> > > > > > > >       int err;
> > > > > > > > +     u32 buflen = 0;
> > > > > > > >
> > > > > > > >       START_USE(vq);
> > > > > > > >
> > > > > > > > @@ -1498,6 +1553,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > > > > >                                       1 << VRING_PACKED_DESC_F_AVAIL |
> > > > > > > >                                       1 << VRING_PACKED_DESC_F_USED;
> > > > > > > >                       }
> > > > > > > > +                     if (n >= out_sgs)
> > > > > > > > +                             buflen += sg->length;
> > > > > > > >               }
> > > > > > > >       }
> > > > > > > >
> > > > > > > > @@ -1518,6 +1575,10 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> > > > > > > >       vq->packed.desc_state[id].last = prev;
> > > > > > > >       vq->packed.desc_state[id].premapped = premapped;
> > > > > > > >
> > > > > > > > +     /* Store in buffer length if necessary */
> > > > > > > > +     if (vq->packed.buflen)
> > > > > > > > +             vq->packed.buflen[id] = buflen;
> > > > > > > > +
> > > > > > > >       /*
> > > > > > > >        * A driver MUST NOT make the first descriptor in the list
> > > > > > > >        * available before all subsequent descriptors comprising
> > > > > > > > @@ -1718,6 +1779,11 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > > > >               BAD_RING(vq, "id %u is not a head!\n", id);
> > > > > > > >               return NULL;
> > > > > > > >       }
> > > > > > > > +     if (vq->packed.buflen && unlikely(*len > vq->packed.buflen[id])) {
> > > > > > > > +             BAD_RING(vq, "used len %d is larger than max in buffer len %u\n",
> > > > > > > > +                     *len, vq->packed.buflen[id]);
> > > > > > > > +             return NULL;
> > > > > > > > +     }
> > > > > > > >
> > > > > > > >       /* detach_buf_packed clears data, so grab it now. */
> > > > > > > >       ret = vq->packed.desc_state[id].data;
> > > > > > > > @@ -1937,6 +2003,7 @@ static void vring_free_packed(struct vring_virtqueue_packed *vring_packed,
> > > > > > > >                                vring_packed->device_event_dma_addr,
> > > > > > > >                                dma_dev);
> > > > > > > >
> > > > > > > > +     kfree(vring_packed->buflen);
> > > > > > > >       kfree(vring_packed->desc_state);
> > > > > > > >       kfree(vring_packed->desc_extra);
> > > > > > > >  }
> > > > > > > > @@ -1988,6 +2055,14 @@ static int vring_alloc_queue_packed(struct vring_virtqueue_packed *vring_packed,
> > > > > > > >
> > > > > > > >       vring_packed->vring.num = num;
> > > > > > > >
> > > > > > > > +     if (vring_needs_used_validation(vdev)) {
> > > > > > > > +             vring_packed->buflen =
> > > > > > > > +                     kmalloc_array(num, sizeof(*vring_packed->buflen),
> > > > > > > > +                                   GFP_KERNEL);
> > > > > > > > +             if (!vring_packed->buflen)
> > > > > > > > +                     goto err;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > >       return 0;
> > > > > > > >
> > > > > > > >  err:
> > > > > > > > --
> > > > > > > > 2.25.1
> > > > > > >
> > > > >
> > >

Powered by blists - more mailing lists