netdev - Re: [PATCH net-next 03/13] virtio_ring: packed: harden dma unmap for indirect

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: <1726126994.8755774-3-xuanzhuo@linux.alibaba.com>
Date: Thu, 12 Sep 2024 15:43:14 +0800
From: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: netdev@...r.kernel.org,
 Jason Wang <jasowang@...hat.com>,
 Eugenio Pérez <eperezma@...hat.com>,
 "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 virtualization@...ts.linux.dev,
 bpf@...r.kernel.org
Subject: Re: [PATCH net-next 03/13] virtio_ring: packed: harden dma unmap for indirect

On Thu, 12 Sep 2024 03:38:12 -0400, "Michael S. Tsirkin" <mst@...hat.com> wrote:
> On Thu, Sep 12, 2024 at 02:55:38PM +0800, Xuan Zhuo wrote:
> > On Wed, 11 Sep 2024 07:28:36 -0400, "Michael S. Tsirkin" <mst@...hat.com> wrote:
> > > As gcc luckily noted:
> > >
> > > On Tue, Aug 20, 2024 at 03:33:20PM +0800, Xuan Zhuo wrote:
> > > > @@ -1617,23 +1617,24 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> > > >  	}
> > > >
> > > >  	if (vq->indirect) {
> > > > +		struct vring_desc_extra *extra;
> > > >  		u32 len;
> > > >
> > > >  		/* Free the indirect table, if any, now that it's unmapped. */
> > > > -		desc = state->indir_desc;
> > > > -		if (!desc)
> > >
> > > desc is no longer initialized here
> >
> >
> > Will fix.
> >
> >
> > >
> > > > +		extra = state->indir;
> > > > +		if (!extra)
> > > >  			return;
> > > >
> > > >  		if (vring_need_unmap_buffer(vq)) {
> > > >  			len = vq->packed.desc_extra[id].len;
> > > >  			for (i = 0; i < len / sizeof(struct vring_packed_desc);
> > > >  					i++)
> > > > -				vring_unmap_desc_packed(vq, &desc[i]);
> > > > +				vring_unmap_extra_packed(vq, &extra[i]);
> > > >  		}
> > > >  		kfree(desc);
> > >
> > >
> > > but freed here
> > >
> > > > -		state->indir_desc = NULL;
> > > > +		state->indir = NULL;
> > > >  	} else if (ctx) {
> > > > -		*ctx = state->indir_desc;
> > > > +		*ctx = state->indir;
> > > >  	}
> > > >  }
> > >
> > >
> > > It seems unlikely this was always 0 on all paths with even
> > > a small amount of stress, so now I question how this was tested.
> > > Besides, do not ignore compiler warnings, and do not tweak code
> > > to just make compiler shut up - they are your friend.
> >
> > I agree.
> >
> > Normally I do this by make W=12, but we have too many message,
> > so I missed this.
> >
> > 	make W=12 drivers/net/virtio_net.o drivers/virtio/virtio_ring.o
> >
> > If not W=12, then I did not get any warning message.
> > How do you get the message quickly?
> >
> > Thanks.
>
>
> If you stress test this for a long enough time, and with
> debug enabled, you will see a crash.

I only stress tested the split ring. For the packed ring, I
just performed a simple verification.

I will street test for two mode in next version.

Thanks.



>
>
> > >
> > > >
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > >
>