Re: [PATCH 1/1] vhost-scsi: Fix handling of multiple calls to vhost_scsi_set_endpoint

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3937 bytes --]

On Tue, Jan 21, 2025 at 03:31:25PM -0600, Mike Christie wrote:
> If vhost_scsi_set_endpoint is called multiple times without a
> vhost_scsi_clear_endpoint between them, we can hit multiple bugs
> found by Haoran Zhang:
> 
> 1. Use-after-free 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.
> 
> 2. Tpg dir removal hang:
> 
> This patch fixes an issue where we cannot remove a LIO/target layer
> tpg (and structs above it like the target) dir due to the refcount
> dropping to -1.
> 
> The problem is that if vhost_scsi_set_endpoint detects a tpg is already
> in the vs->vs_tpg array or if the tpg has been removed so
> target_depend_item fails, the undepend goto handler will do
> target_undepend_item on all tpgs in the vs_tpg array dropping their
> refcount to 0. At this time vs_tpg contains both the tpgs we have added
> in the current vhost_scsi_set_endpoint call as well as tpgs we added in
> previous calls which are also in vs->vs_tpg.
> 
> Later, when vhost_scsi_clear_endpoint runs it will do
> target_undepend_item on all the tpgs in the vs->vs_tpg which will drop
> their refcount to -1. Userspace will then not be able to remove the tpg
> and will hang when it tries to do rmdir on the tpg dir.
> 
> 3. Tpg leak:
> 
> This fixes a bug where we can leak tpgs and cause them to be
> un-removable because the target name is overwritten when
> vhost_scsi_set_endpoint is called multiple times but with different
> target names.
> 
> The bug occurs if a user has called VHOST_SCSI_SET_ENDPOINT and setup
> a vhost-scsi device to target/tpg mapping, then calls
> VHOST_SCSI_SET_ENDPOINT again with a new target name that has tpgs we
> haven't seen before (target1 has tpg1 but target2 has tpg2). When this
> happens we don't teardown the old target tpg mapping and just overwrite
> the target name and the vs->vs_tpg array. Later when we do
> vhost_scsi_clear_endpoint, we are passed in either target1 or target2's
> name and we will only match that target's tpgs when we loop over the
> vs->vs_tpg. We will then return from the function without doing
> target_undepend_item on the tpgs.
> 
> Because of all these bugs, it looks like being able to call
> vhost_scsi_set_endpoint multiple times was never supported. The major
> user, QEMU, already has checks to prevent this use case. So to fix the
> issues, this patch prevents vhost_scsi_set_endpoint from being called
> if it's already successfully added tpgs. To add, remove or change the
> tpg config or target name, you must do a vhost_scsi_clear_endpoint
> first.
> 
> Fixes: 25b98b64e284 ("vhost scsi: alloc cmds per vq instead of session")
> Fixes: 4f7f46d32c98 ("tcm_vhost: Use vq->private_data to indicate if the endpoint is setup")
> Reported-by: Haoran Zhang <wh1sper@zju.edu.cn>
> Closes: https://lore.kernel.org/virtualization/e418a5ee-45ca-4d18-9b5d-6f8b6b1add8e@oracle.com/T/#me6c0041ce376677419b9b2563494172a01487ecb
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/vhost/scsi.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]