[PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

nvmf_create_ctrl() frees opts when ->create_ctrl() returns an error, so
a transport must not free it on its own error paths.  nvme_fc_ctrl_free()
therefore only calls nvmf_free_options() while ctrl->ctrl.opts is still
set, and nvme_fc_init_ctrl() clears that pointer before its last put.

It only does so on the fail_ctrl: path, though.  When nvme_add_ctrl()
fails, nvme_fc_init_ctrl() jumps to out_put_ctrl: instead, so
nvme_fc_ctrl_free() still sees ctrl->ctrl.opts set and frees opts, and
nvmf_create_ctrl() frees it again.

Reproduced with nvme-fcloop and failslab by failing the kvasprintf() in
dev_set_name(), called from nvme_add_ctrl():

  BUG: KASAN: slab-use-after-free in nvmf_free_options+0x30/0x190
   nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
   nvmf_create_ctrl drivers/nvme/host/fabrics.c:1374 [inline]
  Freed by task 5534:
   nvme_fc_ctrl_free drivers/nvme/host/fc.c:2374 [inline]
   nvme_fc_init_ctrl+0xe17/0x1450 drivers/nvme/host/fc.c:3605

Without KASAN opts is simply freed twice.

nvme-tcp and nvme-rdma reach the same error path, but their free_ctrl
only frees opts once the controller is on the global list, so they are
not affected.

Clear ctrl->ctrl.opts on the out_put_ctrl: path as well.  The same
injection then returns -EIO without a report.

Fixes: 1a9e218195a5 ("nvme: split device add from initialization")
Cc: stable@vger.kernel.org
Reported-by: syzbot+f58e57380a6083c4041d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f58e57380a6083c4041d
Assisted-by: Claude:claude-opus-5
Signed-off-by: Rihyeon Kim <rihyeon8648@gmail.com>
---
 drivers/nvme/host/fc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..e4d0eeccd846 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	nvme_uninit_ctrl(&ctrl->ctrl);
 
 out_put_ctrl:
+	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
+	ctrl->ctrl.opts = NULL;
+
 	/* Remove core ctrl ref. */
 	nvme_put_ctrl(&ctrl->ctrl);
 

base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
-- 
2.43.0

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Tue, Aug 11, 2026 at 09:53:10PM +0900, Rihyeon Kim wrote:

(snip)

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489..e4d0eeccd846 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>  	nvme_uninit_ctrl(&ctrl->ctrl);
>  
>  out_put_ctrl:
> +	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
> +	ctrl->ctrl.opts = NULL;
> +
>  	/* Remove core ctrl ref. */
>  	nvme_put_ctrl(&ctrl->ctrl);
>  
> 
> base-commit: 0ce37745d4bfbc493f718169c3974898ffec8ee7
> -- 
> 2.43.0
> 
> 

Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
nvme_tcp_ctrl_free(), i.e. something like:

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..2b4c222995ea 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2359,19 +2359,22 @@ nvme_fc_ctrl_free(struct kref *ref)
                container_of(ref, struct nvme_fc_ctrl, ref);
        unsigned long flags;

+       if (list_empty(&ctrl->ctrl_list))
+               goto free_ctrl;
+
        /* remove from rport list */
        spin_lock_irqsave(&ctrl->rport->lock, flags);
        list_del(&ctrl->ctrl_list);
        spin_unlock_irqrestore(&ctrl->rport->lock, flags);

-       kfree(ctrl->queues);
-
        put_device(ctrl->dev);
        nvme_fc_rport_put(ctrl->rport);

        ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
-       if (ctrl->ctrl.opts)
-               nvmf_free_options(ctrl->ctrl.opts);
+
+       nvmf_free_options(ctrl->ctrl.opts);
+free_ctrl:
+       kfree(ctrl->queues);
        kfree(ctrl);
 }

@@ -3593,8 +3596,6 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
        cancel_work_sync(&ctrl->ctrl.reset_work);
        cancel_delayed_work_sync(&ctrl->connect_work);

-       ctrl->ctrl.opts = NULL;
-
        if (ctrl->ctrl.admin_tagset)
                nvme_remove_admin_tag_set(&ctrl->ctrl);
        /* initiate nvme ctrl ref counting teardown */



However, I'm not familar with the fc driver, so this suggestion is untested.


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Tue, Aug 11, 2026 at 09:53:10PM +0900, Rihyeon Kim wrote:
> It only does so on the fail_ctrl: path, though.  When nvme_add_ctrl()
> fails, nvme_fc_init_ctrl() jumps to out_put_ctrl: instead, so
> nvme_fc_ctrl_free() still sees ctrl->ctrl.opts set and frees opts, and
> nvmf_create_ctrl() frees it again.

...

> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489..e4d0eeccd846 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -3601,6 +3601,9 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
>  	nvme_uninit_ctrl(&ctrl->ctrl);
>  
>  out_put_ctrl:
> +	/* nvme_add_ctrl() failures skip the clear in fail_ctrl: above */
> +	ctrl->ctrl.opts = NULL;
> +
>  	/* Remove core ctrl ref. */
>  	nvme_put_ctrl(&ctrl->ctrl);

Can't you move the setting from the "fail_ctrl:" label to the
"out_put_ctrl:" one instead of duplicating it for both?

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the review.

> Can't you move the setting from the "fail_ctrl:" label to the
> "out_put_ctrl:" one instead of duplicating it for both?

Yes.  I had kept both because I was not sure the put inside
nvme_uninit_ctrl() could not be the last one, which would run
nvme_fc_ctrl_free() before out_put_ctrl: cleared the pointer.  As far as
I could tell nvme-tcp and nvme-rdma use the same uninit-then-put ladder,
and testing does not show it either.

I swept fail-nth 1..200 over the connect write with fcloop and failslab:
unpatched hits the report at 17, and with the clear moved all 200 pass,
21 of the injections landing in nvme_alloc_admin_tag_set(), so fail_ctrl:
is covered as well.

It also looks like moving it drops the window where opts is already NULL
while the fabrics sysfs attributes, which do not check it, are still
there.

v2 on the way.

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the suggestion.

> Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> nvme_tcp_ctrl_free(), i.e. something like:
>
> +       if (list_empty(&ctrl->ctrl_list))
> +               goto free_ctrl;

I am not sure whether that would work, and I may well be missing
something.  From what I could tell, nvme_tcp_create_ctrl() does its
list_add_tail() last, while nvme_fc_init_ctrl() does it before the
nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
failure paths the controller is already on the list and opts would end up
freed twice again.  The early goto would also skip the ida_free(),
put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
before the list_add.

I am not familiar with this driver either, so please correct me if I have
misread it.

Keith suggested moving the existing clear from fail_ctrl: down to
out_put_ctrl: instead, so I will send that as v2.

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the review.

> Can't you move the setting from the "fail_ctrl:" label to the
> "out_put_ctrl:" one instead of duplicating it for both?

Yes.  I had kept both because I was not sure the put inside
nvme_uninit_ctrl() could not be the last one, which would run
nvme_fc_ctrl_free() before out_put_ctrl: cleared the pointer.  As far as
I could tell nvme-tcp and nvme-rdma use the same uninit-then-put ladder,
and testing does not show it either.

I swept fail-nth 1..200 over the connect write with fcloop and failslab:
unpatched hits the report at 17, and with the clear moved all 200 pass,
21 of the injections landing in nvme_alloc_admin_tag_set(), so fail_ctrl:
is covered as well.

It also looks like moving it drops the window where opts is already NULL
while the fabrics sysfs attributes, which do not check it, are still
there.

v2 on the way.

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the suggestion.

> Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> nvme_tcp_ctrl_free(), i.e. something like:
>
> +       if (list_empty(&ctrl->ctrl_list))
> +               goto free_ctrl;

I am not sure whether that would work, and I may well be missing
something.  From what I could tell, nvme_tcp_create_ctrl() does its
list_add_tail() last, while nvme_fc_init_ctrl() does it before the
nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
failure paths the controller is already on the list and opts would end up
freed twice again.  The early goto would also skip the ida_free(),
put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
before the list_add.

I am not familiar with this driver either, so please correct me if I have
misread it.

Keith suggested moving the existing clear from fail_ctrl: down to
out_put_ctrl: instead, so I will send that as v2.

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Wed, Aug 12, 2026 at 07:59:41PM +0900, Rihyeon Kim wrote:
> Hello,
> 
> Thanks for the suggestion.
> 
> > Wouldn't a nicer fix be to change nvme_fc_ctrl_free() to look more like
> > nvme_tcp_ctrl_free(), i.e. something like:
> >
> > +       if (list_empty(&ctrl->ctrl_list))
> > +               goto free_ctrl;
> 
> I am not sure whether that would work, and I may well be missing
> something.  From what I could tell, nvme_tcp_create_ctrl() does its
> list_add_tail() last, while nvme_fc_init_ctrl() does it before the
> nvme_change_ctrl_state() and queue_delayed_work() checks, so on those two
> failure paths the controller is already on the list and opts would end up
> freed twice again.  The early goto would also skip the ida_free(),
> put_device() and nvme_fc_rport_put() for what nvme_fc_alloc_ctrl() takes
> before the list_add.

The fact that both rdma.c and tcp.c does:
1) if (list_empty(ctrl_list)) goto free_ctrl;
2) call list_add() last (after nvme_change_ctrl_state())
3) not have any ctrl->ctrl.opts = NULL; hacks anywhere in them

suggests to me that a proper design would be for the fc.c driver to look
the same as rdma.c and tcp.c.

I'm not familiar with fc.c, but I can imagine that the goto free_ctrl label
can be placed such that ida_free() (and whatever else needs to be called)
is done so after the free_ctrl label, while nvmf_free_options() is done
before the free_ctrl label.

But sure, I agree that such a change would have to be done by someone
familiar with the fc.c driver.


> 
> Keith suggested moving the existing clear from fail_ctrl: down to
> out_put_ctrl: instead, so I will send that as v2.

Looking at the Sashiko comment:
https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com

"Will setting ctrl->opts to NULL here cause a guaranteed NULL pointer
dereference during teardown if DHCHAP authentication is configured?"

makes me even more certain that it is wrong for fc.c to have a
ctrl->ctrl.opts = NULL; before calling nvme_put_ctrl().

The nvme_put_ctrl() call will lead to a call to nvme_free_ctrl(), which will
call e.g. nvme_auth_free(), before calling the ctrl->ops->free_ctrl(ctrl);
callback. So some existing teardown functions in nvme_free_ctrl() expects opts
to be valid/non-NULL, and that the it should be freed earliest by the
ctrl->ops->free_ctrl(ctrl) callback, and for all other fabrics (rdma, fc),
this appears to be true.

Note that the same Sashiko link also reports a sysfs NULL pointer dereference
that is possible by the ctrl->ctrl.opts = NULL;

To me, it seems like fc.c should simply look more like tcp.c and fc.c.

That way:
1) No ctrl->ctrl.opts = NULL; hack needed in fc.c.
2) The sysfs NULL pointer dereference is no longer possible.
3) Teardown functions called by nvme_free_ctrl() can continue to assume that
   ctrl->opts is valid.

Perhaps some of the fc.c maintainers could have a look?


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Wed, Aug 12, 2026 at 04:26:24PM +0200, Niklas Cassel wrote:
> 
> To me, it seems like fc.c should simply look more like tcp.c and fc.c.

[...] more like tcp.c and rdma.c.


Kind regards,
Niklas

[PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

nvmf_create_ctrl() owns the fabrics options and frees them whenever
->create_ctrl() returns an error, so a transport must not free them on
its own error paths.  nvme-fc tracks this by testing ctrl->ctrl.opts in
nvme_fc_ctrl_free(), which requires nvme_fc_init_ctrl() to clear that
pointer on every error exit.

The coupling is implicit, and commit 1a9e218195a5 ("nvme: split device
add from initialization") broke it by adding a second error exit.  When
nvme_add_ctrl() fails, nvme_fc_init_ctrl() jumps to out_put_ctrl:, past
the "ctrl->ctrl.opts = NULL" that only sits on the fail_ctrl: path, so
nvme_fc_ctrl_free() frees the options and nvmf_create_ctrl() frees them
a second time:

  BUG: KASAN: slab-use-after-free in nvmf_free_options+0x30/0x190
   nvmf_free_options+0x30/0x190 drivers/nvme/host/fabrics.c:1284
   nvmf_create_ctrl drivers/nvme/host/fabrics.c:1374 [inline]
  Freed by task 5534:
   nvme_fc_ctrl_free drivers/nvme/host/fc.c:2374 [inline]
   nvme_fc_init_ctrl+0xe17/0x1450 drivers/nvme/host/fc.c:3605

nvme_add_ctrl() fails when dev_set_name() cannot allocate, so this is
reachable under memory pressure or fault injection.  Without KASAN the
options are freed twice.

Rather than clear the pointer on the second exit as well, derive
ownership the way nvme-tcp, nvme-rdma and nvme-loop do, from list
membership: their free_ctrl leaves the options alone unless the
controller made it onto the transport list.

The list cannot simply be populated on the success path as it is there.
nvme-fc runs the initial connect synchronously via flush_delayed_work(),
and the controller has to be reachable on rport->ctrl_list for the whole
of it: nvme_fc_unregister_remoteport() needs to find it to signal
connectivity loss, nvme_fc_match_disconn_ls() matches an incoming
Disconnect Association LS against ctrl->association_id, which is only
assigned during that window, nvme_fc_resume_controller() needs it on
remoteport re-registration, and nvme_fc_existing_controller() uses it to
reject a duplicate connect racing the one in flight.

Keep the insertion where it is and add a fail_unlist: label, falling
into fail_ctrl:, for the error paths that run after it.  The earlier
error paths never reach the insertion and keep using fail_ctrl:
directly, so the list is only touched where the controller is actually
on it.

nvme_fc_ctrl_free() cannot use the plain "goto free_ctrl" the other
transports use, because it still has to put_device(), release the rport
reference and free the ida entry for resources taken before the
insertion.  Sample list_empty() under rport->lock instead.

ctrl->ctrl.opts also stays valid for the whole teardown now.  That is
not the bug being fixed, but it removes some fragility around the old
idiom: nvme_free_ctrl() calls nvme_auth_free() before ->free_ctrl(), and
ctrl_max_dhchaps() dereferences ctrl->opts without a NULL check when
ctrl->dhchap_ctxs is set, which nvme-fc permits since NVMF_ALLOWED_OPTS
allows the dhchap options.  The nvme sysfs attributes that dereference
ctrl->opts, such as hostnqn and address, evaluate their is_visible()
test once at device_add() time and stay readable until
cdev_device_del().

Fixes: 1a9e218195a5 ("nvme: split device add from initialization")
Cc: stable@vger.kernel.org
Reported-by: syzbot+f58e57380a6083c4041d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f58e57380a6083c4041d
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/nvme/host/fc.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..7e1794f054d6 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2358,9 +2358,15 @@ nvme_fc_ctrl_free(struct kref *ref)
 	struct nvme_fc_ctrl *ctrl =
 		container_of(ref, struct nvme_fc_ctrl, ref);
 	unsigned long flags;
+	bool owns_opts;
 
-	/* remove from rport list */
+	/*
+	 * Presence on the rport list means nvme_fc_init_ctrl() completed,
+	 * and with it ownership of the fabrics options passed to it. If it
+	 * failed instead, the options still belong to nvmf_create_ctrl().
+	 */
 	spin_lock_irqsave(&ctrl->rport->lock, flags);
+	owns_opts = !list_empty(&ctrl->ctrl_list);
 	list_del(&ctrl->ctrl_list);
 	spin_unlock_irqrestore(&ctrl->rport->lock, flags);
 
@@ -2370,7 +2376,7 @@ nvme_fc_ctrl_free(struct kref *ref)
 	nvme_fc_rport_put(ctrl->rport);
 
 	ida_free(&nvme_fc_ctrl_cnt, ctrl->cnum);
-	if (ctrl->ctrl.opts)
+	if (owns_opts)
 		nvmf_free_options(ctrl->ctrl.opts);
 	kfree(ctrl);
 }
@@ -3569,14 +3575,14 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 	if (!nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_CONNECTING)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to init ctrl state\n", ctrl->cnum);
-		goto fail_ctrl;
+		goto fail_unlist;
 	}
 
 	if (!queue_delayed_work(nvme_wq, &ctrl->connect_work, 0)) {
 		dev_err(ctrl->ctrl.device,
 			"NVME-FC{%d}: failed to schedule initial connect\n",
 			ctrl->cnum);
-		goto fail_ctrl;
+		goto fail_unlist;
 	}
 
 	flush_delayed_work(&ctrl->connect_work);
@@ -3587,14 +3593,22 @@ nvme_fc_init_ctrl(struct device *dev, struct nvmf_ctrl_options *opts,
 
 	return &ctrl->ctrl;
 
+fail_unlist:
+	/*
+	 * Leaving the list hands the options back to nvmf_create_ctrl();
+	 * see nvme_fc_ctrl_free().  Re-init so that list_empty() there
+	 * reports the controller as unlisted.
+	 */
+	spin_lock_irqsave(&rport->lock, flags);
+	list_del_init(&ctrl->ctrl_list);
+	spin_unlock_irqrestore(&rport->lock, flags);
+
 fail_ctrl:
 	nvme_change_ctrl_state(&ctrl->ctrl, NVME_CTRL_DELETING);
 	cancel_work_sync(&ctrl->ioerr_work);
 	cancel_work_sync(&ctrl->ctrl.reset_work);
 	cancel_delayed_work_sync(&ctrl->connect_work);
 
-	ctrl->ctrl.opts = NULL;
-
 	if (ctrl->ctrl.admin_tagset)
 		nvme_remove_admin_tag_set(&ctrl->ctrl);
 	/* initiate nvme ctrl ref counting teardown */
-- 
2.55.0

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Fri, Aug 14, 2026 at 04:38:34PM +0200, Niklas Cassel wrote:
> ctrl->ctrl.opts also stays valid for the whole teardown now.  That is
> not the bug being fixed, but it removes some fragility around the old
> idiom: nvme_free_ctrl() calls nvme_auth_free() before ->free_ctrl(), and
> ctrl_max_dhchaps() dereferences ctrl->opts without a NULL check when
> ctrl->dhchap_ctxs is set, which nvme-fc permits since NVMF_ALLOWED_OPTS
> allows the dhchap options.  The nvme sysfs attributes that dereference
> ctrl->opts, such as hostnqn and address, evaluate their is_visible()
> test once at device_add() time and stay readable until
> cdev_device_del().

The allocation that ctrl.opts points is freed in this error path, so any
access after that is a use-after-free that needs to be fixed. I think
setting opts to NULL is easier than the proxy locked list check.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Rihyeon Kim]

Hello,

Thanks for the explanation, and for picking this up.

My testing of v1 and v2 never reached this case.  The syzbot config has
"# CONFIG_NVME_HOST_AUTH is not set", and I was not aware that the
dhchap option tokens are compiled out when it is disabled.  A connect
string containing dhchap_secret= is simply rejected, so every run I did
tore down with ctrl->dhchap_ctxs NULL, and there was no path that
dereferenced ctrl->opts.

I rebuilt with CONFIG_NVME_HOST_AUTH=y and swept fail-nth 1..200 over a
connect write containing dhchap_secret=.  You are right:

  clearing opts at out_put_ctrl:   dies at attempt 20
  this patch                       200/200, with 35 of the injections
                                   landing in nvme_add_ctrl()

  Oops: general protection fault, probably for non-canonical address
        0xdffffc0000000008
  KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
  RIP: 0010:nvme_auth_free+0x99/0x450
   nvme_free_ctrl+0x231/0x6c0

The injection for that attempt landed in kobj_map() under cdev_add(), so
this is the nvme_add_ctrl() failure path.  ctrl_max_dhchaps() then
dereferences ctrl->opts at offset 0x40.  Clearing the pointer really does
just trade the double free for this NULL pointer dereference.  Good catch.

Your version also addresses the two things I was unsure about in my
earlier mail: fail_unlist: covers the two exit paths after
list_add_tail(), and checking list_empty() while holding the lock keeps
ida_free(), put_device(), and nvme_fc_rport_put() on the unlisted path.

Dropping my v2 in favour of this one.

Tested-by: Rihyeon Kim <rihyeon8648@gmail.com>

Thanks,
Rihyeon

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

On Mon, Aug 17, 2026 at 03:18:15PM +0900, Rihyeon Kim wrote:
> Hello,
> 
> Thanks for the explanation, and for picking this up.
> 
> My testing of v1 and v2 never reached this case.  The syzbot config has
> "# CONFIG_NVME_HOST_AUTH is not set", and I was not aware that the
> dhchap option tokens are compiled out when it is disabled.  A connect
> string containing dhchap_secret= is simply rejected, so every run I did
> tore down with ctrl->dhchap_ctxs NULL, and there was no path that
> dereferenced ctrl->opts.
> 
> I rebuilt with CONFIG_NVME_HOST_AUTH=y and swept fail-nth 1..200 over a
> connect write containing dhchap_secret=.  You are right:
> 
>   clearing opts at out_put_ctrl:   dies at attempt 20
>   this patch                       200/200, with 35 of the injections
>                                    landing in nvme_add_ctrl()
> 
>   Oops: general protection fault, probably for non-canonical address
>         0xdffffc0000000008
>   KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
>   RIP: 0010:nvme_auth_free+0x99/0x450
>    nvme_free_ctrl+0x231/0x6c0
> 
> The injection for that attempt landed in kobj_map() under cdev_add(), so
> this is the nvme_add_ctrl() failure path.  ctrl_max_dhchaps() then
> dereferences ctrl->opts at offset 0x40.  Clearing the pointer really does
> just trade the double free for this NULL pointer dereference.  Good catch.

Thanks a lot for verifying that the Sashiko comment about the NULL pointer
dereference in nvme_auth_free() can actually happen, and was not only
theoretical.


> 
> Your version also addresses the two things I was unsure about in my
> earlier mail: fail_unlist: covers the two exit paths after
> list_add_tail(), and checking list_empty() while holding the lock keeps
> ida_free(), put_device(), and nvme_fc_rport_put() on the unlisted path.
> 
> Dropping my v2 in favour of this one.
> 
> Tested-by: Rihyeon Kim <rihyeon8648@gmail.com>

It seems like Keith did prefer your patch, but as you said, your patch does
not avoid a NULL pointer dereference in nvme_auth_free().


My patch should also avoid a potential sysfs NULL pointer dereference as
reported by Sashiko:
https://sashiko.dev/#/patchset/20260811125310.165487-1-rihyeon8648%40gmail.com

which is currently also only possible for fc.c, because of the
'ctrl->ctrl.opts = NULL;' hack.


Personally, I prefer this patch, since it makes fc.c more similar to rdma.c,
tcp.c, and loop.c, by checking if the controller is on the linked list or not,
rather than setting 'ctrl->ctrl.opts = NULL;' before the teardown is even done.


I agree that this patch adds more code than what we ideally would have wanted,
but I don't see any other way (other than completely refactoring the ownership
model of the controller options - which would require much bigger changes -
and would most likely require changes to both fabrics.c + all the different
transport drivers).


Kind regards,
Niklas

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Keith Busch]

On Mon, Aug 17, 2026 at 05:10:23PM +0200, Niklas Cassel wrote:
> It seems like Keith did prefer your patch, but as you said, your patch does
> not avoid a NULL pointer dereference in nvme_auth_free().

But you're replacing a NULL pointer dereference to a derefence to freed
memory. That should be fixed too, and I'm just saying checking a pointer
for NULL before dereferencing it is more clear than checking if a list
is empty.

Re: [PATCH] nvme-fc: fix double free of fabrics options when nvme_add_ctrl() fails

[Niklas Cassel]

Hello Keith,

On Wed, Aug 19, 2026 at 09:28:08AM -0600, Keith Busch wrote:
> On Mon, Aug 17, 2026 at 05:10:23PM +0200, Niklas Cassel wrote:
> > It seems like Keith did prefer your patch, but as you said, your patch does
> > not avoid a NULL pointer dereference in nvme_auth_free().
> 
> But you're replacing a NULL pointer dereference to a derefence to freed
> memory. That should be fixed too, and I'm just saying checking a pointer
> for NULL before dereferencing it is more clear than checking if a list
> is empty.

I am not following.

The options is freed in two different places, depending on if the
ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
successful or not.

If the ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl() was
successful, the teardown path will be:
core.c:nvme_free_ctrl(), which calls nvme_auth_free() (which will use
the options), nvme_free_ctrl() will then call ctrl->ops->free_ctrl(ctrl)
which will free the options (since the ops->create_ctrl() call was successful,
the ctrl will be on the linked-list).

If the ops->create_ctrl(dev, opts) call in fabrics.c:nvmf_create_ctrl()
fails, the teardown path will be:
core.c:nvme_free_ctrl(), which calls nvme_auth_free() (which will use
the options), nvme_free_ctrl() will then call ctrl->ops->free_ctrl(ctrl)
which will not free the options (since the ops->create_ctrl() call failed,
the ctrl will not be on the linked-list), fabrics.c:nvmf_create_ctrl() will
thus goto out_module_put, which then does a goto out_free_opts, which
calls nvmf_free_options().

In both cases, nvme_auth_free() will access memory that has not yet been
freed. So I don't see us dereferencing freed memory.


Kind regards,
Niklas