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

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 <[email protected]>

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