[v3,14/16] nvmet-fc: free hostport after release reference to tgtport

Message ID 20231218153105.12717-15-dwagner@suse.de
State New
Headers
Series enable nvmet-fc for blktests |

Commit Message

Daniel Wagner Dec. 18, 2023, 3:31 p.m. UTC
  Give the ref back before destroying the hostport object to prevent a
potential UAF.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 drivers/nvme/target/fc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Hannes Reinecke Dec. 19, 2023, 11:41 a.m. UTC | #1
On 12/18/23 16:31, Daniel Wagner wrote:
> Give the ref back before destroying the hostport object to prevent a
> potential UAF.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   drivers/nvme/target/fc.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 663c51c9fe53..23d8779dc221 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -986,8 +986,8 @@ nvmet_fc_hostport_free(struct kref *ref)
>   	spin_unlock_irqrestore(&tgtport->lock, flags);
>   	if (tgtport->ops->host_release && hostport->invalid)
>   		tgtport->ops->host_release(hostport->hosthandle);
> -	kfree(hostport);
>   	nvmet_fc_tgtport_put(tgtport);
> +	kfree(hostport);
>   }
>   
>   static void

That, I guess, needs some more explanation.
It's not immediately obvious why a 'put' on the targetport would
have implications on the hostport. But when it does wouldn't it
be more sensible to free up the reference to the hostport when the
final 'put' on the target port happens?

Cheers,

Hannes
  

Patch

diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 663c51c9fe53..23d8779dc221 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -986,8 +986,8 @@  nvmet_fc_hostport_free(struct kref *ref)
 	spin_unlock_irqrestore(&tgtport->lock, flags);
 	if (tgtport->ops->host_release && hostport->invalid)
 		tgtport->ops->host_release(hostport->hosthandle);
-	kfree(hostport);
 	nvmet_fc_tgtport_put(tgtport);
+	kfree(hostport);
 }
 
 static void