Don't pass a user buffer pointer as the data pointer in a CCB.
ClosedPublic
Actions

Authored by jhb on Apr 18 2020, 8:22 PM.

Details

Reviewers

scottl
imp
brooks

Commits

rS360179: Don't pass a user buffer pointer as the data pointer in a CCB.

Summary

Allocate a temporary buffer in the kernel to serve as the CCB data
pointer for a pass-through transaction and using copyin/copyout to
shuffle the data to/from the user buffer.

Test Plan

compile tested only, found on CheriBSD where user and kernel pointers have different types, so conflating the two gives compile errors

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 30596
Build 28337: arc lint + arc unit

Event Timeline

jhb created this revision.Apr 18 2020, 8:22 PM

Harbormaster completed remote builds in B30596: Diff 70750.Apr 18 2020, 8:22 PM

imp added inline comments.Apr 18 2020, 9:15 PM

sys/cam/scsi/scsi_sg.c
633	Dir & CAM_DIR_OUT?

Modulo accepting or rejecting @imp's suggestion this looks good.

This revision is now accepted and ready to land.Apr 20 2020, 5:08 PM

jhb added inline comments.Apr 20 2020, 10:16 PM

sys/cam/scsi/scsi_sg.c

582

That is why I changed this btw, because BOTH != IN | OUT.

633

If only that would work. :(

From cam.h (and I don't know why this is so dumb):

	CAM_DIR_BOTH		= 0x00000000,/* Data direction (00:IN/OUT)    */
	CAM_DIR_IN		= 0x00000040,/* Data direction (01:DATA IN)   */
	CAM_DIR_OUT		= 0x00000080,/* Data direction (10:DATA OUT)  */
	CAM_DIR_NONE		= 0x000000C0,/* Data direction (11:no data)   */
	CAM_DIR_MASK		= 0x000000C0,/* Data direction Mask	      */

scottl added inline comments.Apr 21 2020, 3:12 PM

sys/cam/scsi/scsi_sg.c
633	Then we should seriously consider changing this.

jhb added a subscriber: ken.Apr 21 2020, 5:51 PM

jhb added inline comments.

sys/cam/scsi/scsi_sg.c
633	Hmm, rS216088 is when the 0 value was changed from RESERVED to BOTH. Maybe @ken has some thoughts?

I do think if we change CAM_DIR_BOTH we should probably do that as a separate change from this though? Do we think this patch is ok given the current CAM_DIR_* values?

Yep, can tackle the CAM_DIR problem separately.

ken added inline comments.Apr 21 2020, 6:06 PM

sys/cam/scsi/scsi_sg.c
633	I added CAM_DIR_BOTH for SMP commands, because they are effectively bidirectional. The request was too big to allocate statically in the CCB. The SCSI standard also defines bi-directional commands, at least with opcode 0x9D (Service Action Bi-Directional). In other words, we can't remove bi-directional support, we need it in some form. This is also why it is typically written: if ((ccb->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_X)

The problem isn't the existence of bi-directional commands, it's that DIR_BOTH == 0, not an OR'ing of DIR_IN and DIR_OUT, as would be expected.

In D24489#539455, @scottl wrote:

The problem isn't the existence of bi-directional commands, it's that DIR_BOTH == 0, not an OR'ing of DIR_IN and DIR_OUT, as would be expected.

Well, CAM_DIR_NONE was defined as all 1's from the beginning. It was that way in the ANSI CAM spec. 0's was defined as CAM_DIR_RESV.

So that is why it is that way...

Blah, but thanks for the history on it, I guess I never noticed before. I would be surprised if this hasn't caused bugs. There's no longer an active standards group, and even if there was I'd advocate that this is wrong. Let's follow up with a change on it.

In D24489#539551, @scottl wrote:

Blah, but thanks for the history on it, I guess I never noticed before. I would be surprised if this hasn't caused bugs. There's no longer an active standards group, and even if there was I'd advocate that this is wrong. Let's follow up with a change on it.

I'd love to change this too... but I worry about userland/kernel ABI changes being hard to transition...

Closed by commit rS360179: Don't pass a user buffer pointer as the data pointer in a CCB. (authored by jhb). · Explain WhyApr 21 2020, 11:39 PM

This revision was automatically updated to reflect the committed changes.

jhb added a commit: rS360179: Don't pass a user buffer pointer as the data pointer in a CCB..