linux-kernel - Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210203164551.GG74271@stefanha-x1.localdomain>
Date:   Wed, 3 Feb 2021 16:45:51 +0000
From:   Stefan Hajnoczi <stefanha@...hat.com>
To:     Stefano Garzarella <sgarzare@...hat.com>
Cc:     virtualization@...ts.linux-foundation.org,
        Xie Yongji <xieyongji@...edance.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Laurent Vivier <lvivier@...hat.com>,
        linux-kernel@...r.kernel.org, Max Gurtovoy <mgurtovoy@...dia.com>,
        Jason Wang <jasowang@...hat.com>, kvm@...r.kernel.org
Subject: Re: [PATCH RFC v2 08/10] vdpa: add vdpa simulator for block device

On Tue, Feb 02, 2021 at 04:49:50PM +0100, Stefano Garzarella wrote:
> On Tue, Feb 02, 2021 at 09:34:12AM +0000, Stefan Hajnoczi wrote:
> > On Thu, Jan 28, 2021 at 03:41:25PM +0100, Stefano Garzarella wrote:
> > > +static void vdpasim_blk_work(struct work_struct *work)
> > > +{
> > > +	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > > +	u8 status = VIRTIO_BLK_S_OK;
> > > +	int i;
> > > +
> > > +	spin_lock(&vdpasim->lock);
> > > +
> > > +	if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
> > > +		goto out;
> > > +
> > > +	for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
> > > +		struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
> > > +
> > > +		if (!vq->ready)
> > > +			continue;
> > > +
> > > +		while (vringh_getdesc_iotlb(&vq->vring, &vq->out_iov,
> > > +					    &vq->in_iov, &vq->head,
> > > +					    GFP_ATOMIC) > 0) {
> > > +			int write;
> > > +
> > > +			vq->in_iov.i = vq->in_iov.used - 1;
> > > +			write = vringh_iov_push_iotlb(&vq->vring, &vq->in_iov,
> > > +						      &status, 1);
> > > +			if (write <= 0)
> > > +				break;
> > 
> > This code looks fragile:
> > 
> > 1. Relying on unsigned underflow and the while loop in
> >   vringh_iov_push_iotlb() to handle the case where in_iov.used == 0 is
> >   risky and could break.
> > 
> > 2. Does this assume that the last in_iov element has size 1? For
> >   example, the guest driver may send a single "in" iovec with size 513
> >   when reading 512 bytes (with an extra byte for the request status).
> > 
> > Please validate inputs fully, even in test/development code, because
> > it's likely to be copied by others when writing production code (or
> > deployed in production by unsuspecting users) :).
> 
> Perfectly agree on that, so I addressed these things, also following your
> review on the previous version, on the next patch of this series:
> "vdpa_sim_blk: implement ramdisk behaviour".
> 
> Do you think should I move these checks in this patch?
> 
> I did this to leave Max credit for this patch and add more code to emulate a
> ramdisk in later patches.

You could update the commit description so it's clear that input
validation is missing and will be added in the next commit.

Stefan

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)