iscsi: Always free a cdw before its associated ctl_io.
ClosedPublic
Actions

Authored by jhb on May 14 2021, 10:55 PM.

Details

Reviewers

np
mav
trasz

Group Reviewers

cam

Commits

rGb77045f1ca6a: iscsi: Always free a cdw before its associated ctl_io.
rG71e3d1b3a0ee: iscsi: Always free a cdw before its associated ctl_io.

Summary

cxgbei stores state about a target transfer in the ctl_private[] array
of a ctl_io that is freed when a target transfer (represented by the
cdw) is freed. As such, freeing a ctl_io before a cdw that references
it can result in a use after free in cxgbei. Two of the four places
freed the cdw first, and the other two freed the ctl_io first. Fix
the latter two places to free the cdw first.

Reported by: Jithesh Arakkan @ Chelsio

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 39201
Build 36090: arc lint + arc unit

Event Timeline

jhb created this revision.May 14 2021, 10:55 PM

Herald added a reviewer: cam. · View Herald TranscriptMay 14 2021, 10:55 PM

Herald added a subscriber: imp. · View Herald Transcript

jhb requested review of this revision.May 14 2021, 10:55 PM

Harbormaster completed remote builds in B39201: Diff 89250.May 14 2021, 10:55 PM

The panics showed up after Chelsio QA started testing the fix in e894e3adb206815c2acff17a4011becb166c2f66. Specifically, in some cases due to the bug this change fixes, the write to clear the pointer to NULL in ctl_io in the cxgbei driver in that commit would write to ctl_io after it had been freed by ctl_datamove_done().

sys/cam/ctl/ctl_frontend_iscsi.c
2959	I would appreciate some thoughts on my questions here.

Looks good to me. Just remove the XXX comment.

sys/cam/ctl/ctl_frontend_iscsi.c
2959	No, cdw_io != io here. io is the abort request, while cdw_io is the original SCSI command being aborted. You've properly fixed CTL_FLAG_DMA_INPROG clear on wrong io.

imp added inline comments.May 15 2021, 2:46 PM

sys/cam/ctl/ctl_frontend_iscsi.c
2967	above we set it to 42, here 43? I'm not super familiar with the code, but is that a problem?

mav added inline comments.May 15 2021, 3:17 PM

sys/cam/ctl/ctl_frontend_iscsi.c
2967	These codes were never formalized. I've done some cleanup, but haven't completed. So anything non-zero is OK, unique values are used just to ease debugging. And since the command was aborted, this code goes nowhere really, but still it is better to indicate that the transfer was not successful.