Re: [PATCH v3] scsi: elx: efct: fix target session lifetime against node teardown

[Yehyeong Lee]

Please drop this patch. The node use after free it describes is real, but I
no longer think a correct fix fits in efct_lio.c alone, and I would rather
withdraw it than have a partial fix carry a Fixes tag. What follows is what
I found, in case it is useful. Line numbers are against f47d223627f8.

efct_scsi_new_initiator() hands a bare struct efc_node to
efct_lio_setup_session() through lio_wq and returns EFC_SCSI_CALL_ASYNC, so
the node state machine keeps running while the work is queued. A peer LOGO,
an FC link down, a fabric change or target teardown during that window frees
the node under the work, which then reads it:

  BUG: KASAN: slab-use-after-free in efct_lio_setup_session+0x934/0xa30
  Read of size 8 at addr ffff888002d538a0 by task kworker/u4:0/12

  CPU: 0 UID: 0 PID: 12 Comm: kworker/u4:0 Tainted: G                 N  7.3.0-00342-g66b1ae16db8c-dirty #7 PREEMPT(lazy)
  Tainted: [N]=TEST
  Workqueue: efct-kunit-wq efct_lio_setup_session
  Call Trace:
   <TASK>
   print_report+0xcd/0x620
   ? efct_lio_setup_session+0x934/0xa30
   ? __virt_addr_valid+0x208/0x3f0
   ? efct_lio_setup_session+0x934/0xa30
   kasan_report+0xfb/0x130
   ? efct_lio_setup_session+0x934/0xa30
   efct_lio_setup_session+0x934/0xa30
   ? __pfx_efct_lio_setup_session+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? process_one_work+0x835/0x1c40
   ? process_one_work+0x735/0x1c40
   ? lock_release+0xc3/0x250
   process_one_work+0x88b/0x1c40
   ? __pfx_process_one_work+0x10/0x10
   ? lock_acquire+0x100/0x2d0
   ? worker_thread+0x2c7/0x11b0
   ? lock_is_held_type+0x84/0xf0
   ? __pfx_efct_lio_setup_session+0x10/0x10
   worker_thread+0x649/0x11b0
   ? __pfx_worker_thread+0x10/0x10
   kthread+0x3aa/0x550
   ? _raw_spin_unlock_irq+0x23/0x40
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x493/0x940
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdc0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

  Allocated by task 33:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_noprof+0x23b/0x580
   mempool_alloc_noprof+0x143/0x230
   efct_kt_node_alloc.isra.0+0x38/0x4b0
   efct_kt_uaf_after_cb+0x7a/0x370
   kunit_try_run_case+0x1ca/0x4c0
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  Freed by task 12:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   kasan_save_free_info+0x3b/0x60
   __kasan_slab_free+0x43/0x70
   kfree+0x119/0x490
   mempool_free+0xbc/0x100
   efc_node_free+0x311/0x4a0
   efct_lio_setup_session+0x6f1/0xa30
   process_one_work+0x88b/0x1c40
   worker_thread+0x649/0x11b0
   kthread+0x3aa/0x550
   ret_from_fork+0x493/0x940
   ret_from_fork_asm+0x19/0x30

  The buggy address belongs to the object at ffff888002d53800
   which belongs to the cache kmalloc-1k of size 1024
  The buggy address is located 160 bytes inside of
   freed 1024-byte region [ffff888002d53800, ffff888002d53c00)

Taking a reference on the node for the life of the work stops the slab use
after free, but it does not stop the driver faulting on the same ordering.
efc_node_free() clears node->nport before it drops the reference, so
efct_get_vport_tpg() and efct_session_cb() still dereference a NULL nport
unless they check it under efc->lock. I measured that: with the reference
held and nothing else changed, the same case moves from a
slab-use-after-free to a NULL pointer dereference in efct_session_cb().

The reason I stopped is one step below that. libefc does not guarantee the
backend is told to delete a session before it frees the node that owns it.
efc_node_free() makes no backend upcall at all, and scsi_del_node is reached
only from __efc_d_initiate_shutdown(), efc_device.c:225, :241, :255 and
:264, and from __efc_d_device_gone(), efc_device.c:1451 to :1475. The first
of those picks a branch from node->init and node->targ, and when both are
false it falls through to efc_device.c:293 and goes to cleanup with no upcall
at all. A remote initiator can arrange that, because
efc_process_prli_payload() takes both flags straight from the PRLI service
parameters at efc_device.c:366, and __efc_d_device_ready() accepts a further
PRLI and re-runs that at efc_device.c:1355, after a session is registered.
__efc_node_paused() is a second such path, setting node->req_free on
EFC_EVT_SHUTDOWN at efc_node.c:916 with no upcall either. On both, a
registered se_session and its efct->lookup entry outlive the struct efc_node
they belong to, and efct_node_find(), efct_unsol.c:22, hands that entry out
and takes a reference on it at efct_unsol.c:34 for every inbound FCP or BLS
frame.

Three smaller things nearby, which I am not proposing to fix either. The
node->refound block in efc_node_free() calls
efc_node_find(node->nport, FC_FID_DIR_SERV) at efc_node.c:137, above the
"Node already Freed" guard at efc_node.c:140 that would have caught a NULL
nport. __efc_d_device_ready() discards the scsi_new_node() return value at
efc_device.c:1312, so an EFC_SCSI_CALL_ASYNC there is never completed. And
the target branch of __efc_d_initiate_shutdown() tests only
rc == EFC_SCSI_CALL_COMPLETE at efc_device.c:244, :258 and :267, while the
initiator branch at :228 also accepts a negative return, so a negative
return in the target branch leaves the node in __efc_d_wait_del_node,
efc_device.c:82, with nothing left to wake it.

What I think it wants is both halves together, a reference held on the node
for the life of the setup work, and an invariant in libefc that a node for
which scsi_new_node() succeeded gets exactly one scsi_del_node() before
efc_node_free(), independent of init and targ. The second half changes the
node state machine contract, including how the two delete completion events
pair up and what a re-attach does, so that is your call and not something I
should decide from the backend. I have a KUnit harness that reproduces the
use after free, the session leak and the stale lookup entry, and I am happy
to write patches or test whatever shape you prefer.