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

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

Thanks,
Rihyeon