Re: [PATCH] vhost/scsi: Fix improper cleanup in vhost_scsi_set_endpoint()

[Mike Christie <michael.christie@oracle.com>]

On 1/17/25 5:42 AM, Haoran Zhang wrote:
>> I see now and can replicate it. I think there is a 2nd bug in
>> vhost_scsi_set_endpoint related to all this where we need to
>> prevent switching targets like this or else we'll leak some
>> other refcounts. If 500140501e23be28's tpg number was 3 then
>> we would overwrite the existing vs->vs_vhost_wwpn and never
>> be able to release the refounts on the tpgs from 500140562c8936fa.
>>
>> I'll send a patchset to fix everything and cc you.
>>
>> Thanks for all the work you did testing and debugging this
>> issue.
> 
> You are welcome. There is another bug I was about to report, but I'm not
> sure whether I should create a new thread. I feel that the original design
> of dynamically allocating new vs_tpgs in vhost_scsi_set_endpoint is not
> intuitive, and copying TPGs before setting the target doesn't seem
> logical. Since you are already refactoring the code, maybe I should post
> it here so we can address these issues in one go.

Yeah, I'm not sure if being able to call vhost_scsi_set_endpoint multiple
times and pick up new tpgs is actually a feature or not. There's so many
bugs and it also doesn't support tpg removal.

> 
> [PATCH] vhost/scsi: Fix dangling pointer in vhost_scsi_set_endpoint()
> 
> Since commit 4f7f46d32c98 ("tcm_vhost: Use vq->private_data to indicate
> if the endpoint is setup"), a dangling pointer issue has been introduced
> in vhost_scsi_set_endpoint() when the host fails to reconfigure the
> vhost-scsi endpoint. Specifically, this causes a UAF fault in
> vhost_scsi_get_req() when the guest attempts to send an SCSI request.
> 
I saw that while reviewing the code. Here is my patch. I just added a new
goto, because we don't need to do the undepend since we never did any
depend calls.

--------------------


From 0474c5d41968095ea911d48159e4f6a129f1a862 Mon Sep 17 00:00:00 2001
From: Mike Christie <michael.christie@oracle.com>
Date: Wed, 15 Jan 2025 19:05:22 -0600
Subject: [PATCH 1/3] vhost-scsi: Avoid accessing a freed vs_tpg when no tpgs
 are found

This fixes a use after free that occurs when vhost_scsi_set_endpoint is
called more than once and calls after the first call do not find any
tpgs to add to the vs_tpg. When vhost_scsi_set_endpoint first finds
tpgs to add to the vs_tpg array match=true, so we will do:

vhost_vq_set_backend(vq, vs_tpg);
...

kfree(vs->vs_tpg);
vs->vs_tpg = vs_tpg;

If vhost_scsi_set_endpoint is called again and no tpgs are found
match=false so we skip the vhost_vq_set_backend call leaving the
pointer to the vs_tpg we then free via:

kfree(vs->vs_tpg);
vs->vs_tpg = vs_tpg;

If a scsi request is then sent we do:

vhost_scsi_handle_vq -> vhost_scsi_get_req -> vhost_vq_get_backend

which sees the vs_tpg we just did a kfree on.

This fixes the issue by having us not reset and free the existing
vs->vs-tpg pointer so the virtqueue private_data stays valid.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/vhost/scsi.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 718fa4e0b31e..143276df16e2 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -1775,6 +1775,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
 		ret = 0;
 	} else {
 		ret = -EEXIST;
+		goto free_vs_tpg;
 	}
 
 	/*
@@ -1802,6 +1803,7 @@ vhost_scsi_set_endpoint(struct vhost_scsi *vs,
 			target_undepend_item(&tpg->se_tpg.tpg_group.cg_item);
 		}
 	}
+free_vs_tpg:
 	kfree(vs_tpg);
 out:
 	mutex_unlock(&vs->dev.mutex);
-- 
2.43.0