On 2018/11/2 下午6:50, Stefan Hajnoczi wrote:
If the network stack calls .send_pkt()/.cancel_pkt() during .release(),
a struct vhost_vsock use-after-free is possible. This occurs because
.release() does not wait for other CPUs to stop using struct
vhost_vsock.
Switch to an RCU-enabled hashtable (indexed by guest CID) so that
.release() can wait for other CPUs by calling synchronize_rcu(). This
also eliminates vhost_vsock_lock acquisition in the data path so it
could have a positive effect on performance.
Reported-and-tested-by: syzbot+bd391451452fb0b93039@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+e3e074963495f92a89ed@xxxxxxxxxxxxxxxxxxxxxxxxx
Reported-by: syzbot+d5a0a170c5069658b141@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
---
Hi Jason,
Thanks for encouraging me to try the RCU hashtable approach. This patch
passes syzbot but I'm going to do some manual testing now. Thoughts?
drivers/vhost/vsock.c | 57 +++++++++++++++++++++++++------------------
1 file changed, 33 insertions(+), 24 deletions(-)
[...]
out:
@@ -577,9 +577,13 @@ static int vhost_vsock_dev_release(struct inode *inode, struct file *file)
struct vhost_vsock *vsock = file->private_data;
spin_lock_bh(&vhost_vsock_lock);
It looks to me there's no need to use bh locks here.
Other looks good.
Thanks
- list_del(&vsock->list);
+ if (vsock->guest_cid)
+ hash_del_rcu(&vsock->hash);
spin_unlock_bh(&vhost_vsock_lock);
+ /* Wait for other CPUs to finish using vsock */
+ synchronize_rcu();
+
/* Iterating over all connections for all CIDs to find orphans is
* inefficient. Room for improvement here. */
vsock_for_each_connected_socket(vhost_vsock_reset_orphans);
@@ -620,12 +624,17 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
/* Refuse if CID is already in use */
spin_lock_bh(&vhost_vsock_lock);
- other = __vhost_vsock_get(guest_cid);
+ other = vhost_vsock_get(guest_cid);
if (other && other != vsock) {
spin_unlock_bh(&vhost_vsock_lock);
return -EADDRINUSE;
}
+
+ if (vsock->guest_cid)
+ hash_del_rcu(&vsock->hash);
+
vsock->guest_cid = guest_cid;
+ hash_add_rcu(vhost_vsock_hash, &vsock->hash, guest_cid);
spin_unlock_bh(&vhost_vsock_lock);
return 0;
