Page MenuHomeFreeBSD

mpi3mr: Prevent use-after-free during concurrent drive removal and host I/O
Needs ReviewPublic

Authored by chandrakanth.patil_broadcom.com on Sun, Oct 4, 12:30 PM.

Details

Summary

When a drive or RAID volume was removed or deleted while host I/O was
actively in progress, the driver polled for pending completions only
once. If outstanding requests had not completed within that single poll,
the driver proceeded to remove and free the target structure. When those
in-flight I/Os subsequently completed, dereferencing the stale target
pointer resulted in a use-after-free condition.

Drain pending I/Os cleanly before completing device removal by waiting
up to two seconds while repeatedly polling for completions. In the I/O
completion path, safely look up the target from the active list and guard
all dereferences with null checks rather than accessing the freed pointer
directly.

Test Plan
  • Clean build with WERROR=-Werror across FreeBSD 16, 15, and 14 with INVARIANTS/WITNESS enabled; git bisect verified.
  • Tested hot-unplug of drives and dynamic deletion of RAID virtual disks (via StorCLI2) during heavy concurrent FIO workloads on SAS4116/SAS5116 controllers.
  • Verified that in-flight I/Os drain cleanly during removal and completions finish without use-after-free or memory corruption.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

sys/dev/mpi3mr/mpi3mr.c
4889

tg == NULL means that target may be NULL since we don't set tg unless target is NULL)

4993

How do we know that target == cm->targ here?

sys/dev/mpi3mr/mpi3mr_cam.c
1939

So normally the outstanding count protects things, but if we get here with outstanding != 0, so if there's a concurrent interrupt running process_op_reply_desc which looks up a target, then that thread's use of target will race the mpi3mr_remove_device_from_list() that follows the calls to mpi3mr_remove_device_from_os(). Right?

sys/dev/mpi3mr/mpi3mr.c
4889

tg == NULL means that target may be NULL since we don't set tg unless target is NULL)

target cannot be NULL here because this block is entered only when throttle_enabled_dev is non-zero, which requires target != NULL. tg is NULL for physical drives (PDs) because only RAID volumes (VDs) belong to a throttle group.

4993

How do we know that target == cm->targ here?

target_id is obtained from csio->ccb_h.target_id and looked up via mpi3mr_find_target_by_per_id(). If the target is still present, it matches cm->targ. If the drive was removed while the I/O was in-flight, target evaluates to NULL, and the code safely sets CAM_DEV_NOT_THERE without dereferencing cm->targ.

sys/dev/mpi3mr/mpi3mr_cam.c
1939

So normally the outstanding count protects things, but if we get here with outstanding != 0, so if there's a concurrent interrupt running process_op_reply_desc which looks up a target, then that thread's use of target will race the mpi3mr_remove_device_from_list() that follows the calls to mpi3mr_remove_device_from_os(). Right?

Yes, that is correct. If the drain loop exits with outstanding != 0, freeing the target immediately would race with the completion path. I will update mpi3mr_remove_device_from_list() in V2 patch to ensure the target is not freed while target->outstanding > 0.