| Message ID | 20220516084213.26854-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vhost_net: fix double fget() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Mon, May 16, 2022 at 04:42:13PM +0800, Jason Wang wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Here's another piece of code assuming that repeated fget() will yield the > same opened file: in vhost_net_set_backend() we have > > sock = get_socket(fd); > if (IS_ERR(sock)) { > r = PTR_ERR(sock); > goto err_vq; > } > > /* start polling new socket */ > oldsock = vhost_vq_get_backend(vq); > if (sock != oldsock) { > ... > vhost_vq_set_backend(vq, sock); > ... > if (index == VHOST_NET_VQ_RX) > nvq->rx_ring = get_tap_ptr_ring(fd); > > with > static struct socket *get_socket(int fd) > { > struct socket *sock; > > /* special case to disable backend */ > if (fd == -1) > return NULL; > sock = get_raw_socket(fd); > if (!IS_ERR(sock)) > return sock; > sock = get_tap_socket(fd); > if (!IS_ERR(sock)) > return sock; > return ERR_PTR(-ENOTSOCK); > } > and > static struct ptr_ring *get_tap_ptr_ring(int fd) > { > struct ptr_ring *ring; > struct file *file = fget(fd); > > if (!file) > return NULL; > ring = tun_get_tx_ring(file); > if (!IS_ERR(ring)) > goto out; > ring = tap_get_ptr_ring(file); > if (!IS_ERR(ring)) > goto out; > ring = NULL; > out: > fput(file); > return ring; > } > > Again, there is no promise that fd will resolve to the same thing for > lookups in get_socket() and in get_tap_ptr_ring(). I'm not familiar > enough with the guts of drivers/vhost to tell how easy it is to turn > into attack, but it looks like trouble. If nothing else, the pointer > returned by tun_get_tx_ring() is not guaranteed to be pinned down by > anything - the reference to sock will _usually_ suffice, but that > doesn't help any if we get a different socket on that second fget(). > > One possible way to fix it would be the patch below; objections? > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > Signed-off-by: Jason Wang <jasowang@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> and this is stable material I guess. > --- > drivers/vhost/net.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c > index 28ef323882fb..0bd7d91de792 100644 > --- a/drivers/vhost/net.c > +++ b/drivers/vhost/net.c > @@ -1449,13 +1449,9 @@ static struct socket *get_raw_socket(int fd) > return ERR_PTR(r); > } > > -static struct ptr_ring *get_tap_ptr_ring(int fd) > +static struct ptr_ring *get_tap_ptr_ring(struct file *file) > { > struct ptr_ring *ring; > - struct file *file = fget(fd); > - > - if (!file) > - return NULL; > ring = tun_get_tx_ring(file); > if (!IS_ERR(ring)) > goto out; > @@ -1464,7 +1460,6 @@ static struct ptr_ring *get_tap_ptr_ring(int fd) > goto out; > ring = NULL; > out: > - fput(file); > return ring; > } > > @@ -1551,8 +1546,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) > r = vhost_net_enable_vq(n, vq); > if (r) > goto err_used; > - if (index == VHOST_NET_VQ_RX) > - nvq->rx_ring = get_tap_ptr_ring(fd); > + if (index == VHOST_NET_VQ_RX) { > + if (sock) > + nvq->rx_ring = get_tap_ptr_ring(sock->file); > + else > + nvq->rx_ring = NULL; > + } > > oldubufs = nvq->ubufs; > nvq->ubufs = ubufs; > -- > 2.25.1
On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote: > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > and this is stable material I guess. It is, except that commit message ought to be cleaned up. Something along the lines of ---- Fix double fget() in vhost_net_set_backend() Descriptor table is a shared resource; two fget() on the same descriptor may return different struct file references. get_tap_ptr_ring() is called after we'd found (and pinned) the socket we'll be using and it tries to find the private tun/tap data structures associated with it. Redoing the lookup by the same file descriptor we'd used to get the socket is racy - we need to same struct file. Thanks to Jason for spotting a braino in the original variant of patch - I'd missed the use of fd == -1 for disabling backend, and in that case we can end up with sock == NULL and sock != oldsock. ---- Does the above sound sane for commit message? And which tree would you prefer it to go through? I can take it in vfs.git#fixes, or you could take it into your tree...
On Wed, May 18, 2022 at 6:00 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote: > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > and this is stable material I guess. > > It is, except that commit message ought to be cleaned up. Something > along the lines of > > ---- > Fix double fget() in vhost_net_set_backend() > > Descriptor table is a shared resource; two fget() on the same descriptor > may return different struct file references. get_tap_ptr_ring() is > called after we'd found (and pinned) the socket we'll be using and it > tries to find the private tun/tap data structures associated with it. > Redoing the lookup by the same file descriptor we'd used to get the > socket is racy - we need to same struct file. > > Thanks to Jason for spotting a braino in the original variant of patch - > I'd missed the use of fd == -1 for disabling backend, and in that case > we can end up with sock == NULL and sock != oldsock. > ---- > > Does the above sound sane for commit message? Yes. > And which tree would you > prefer it to go through? I can take it in vfs.git#fixes, or you could > take it into your tree... > Consider Michael gave an ack, it would be fine if you want to take via your tree. Thanks
On Tue, May 17, 2022 at 10:00:03PM +0000, Al Viro wrote: > On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote: > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > Acked-by: Michael S. Tsirkin <mst@redhat.com> > > > > and this is stable material I guess. > > It is, except that commit message ought to be cleaned up. Something > along the lines of > > ---- > Fix double fget() in vhost_net_set_backend() > > Descriptor table is a shared resource; two fget() on the same descriptor > may return different struct file references. get_tap_ptr_ring() is > called after we'd found (and pinned) the socket we'll be using and it > tries to find the private tun/tap data structures associated with it. > Redoing the lookup by the same file descriptor we'd used to get the > socket is racy - we need to same struct file. > > Thanks to Jason for spotting a braino in the original variant of patch - > I'd missed the use of fd == -1 for disabling backend, and in that case > we can end up with sock == NULL and sock != oldsock. > ---- > > Does the above sound sane for commit message? And which tree would you > prefer it to go through? I can take it in vfs.git#fixes, or you could > take it into your tree... Acked-by: Michael S. Tsirkin <mst@redhat.com> for the new message and merging through your tree.
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c index 28ef323882fb..0bd7d91de792 100644 --- a/drivers/vhost/net.c +++ b/drivers/vhost/net.c @@ -1449,13 +1449,9 @@ static struct socket *get_raw_socket(int fd) return ERR_PTR(r); } -static struct ptr_ring *get_tap_ptr_ring(int fd) +static struct ptr_ring *get_tap_ptr_ring(struct file *file) { struct ptr_ring *ring; - struct file *file = fget(fd); - - if (!file) - return NULL; ring = tun_get_tx_ring(file); if (!IS_ERR(ring)) goto out; @@ -1464,7 +1460,6 @@ static struct ptr_ring *get_tap_ptr_ring(int fd) goto out; ring = NULL; out: - fput(file); return ring; } @@ -1551,8 +1546,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd) r = vhost_net_enable_vq(n, vq); if (r) goto err_used; - if (index == VHOST_NET_VQ_RX) - nvq->rx_ring = get_tap_ptr_ring(fd); + if (index == VHOST_NET_VQ_RX) { + if (sock) + nvq->rx_ring = get_tap_ptr_ring(sock->file); + else + nvq->rx_ring = NULL; + } oldubufs = nvq->ubufs; nvq->ubufs = ubufs;