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.
--------------------
