Don't allocate a stack buffer with a user-controlled size for CAMIOCOMMAND.

[jhb]

Summary

Restrict the size of the CDB buffer to IOCDBLEN similar to CAMIOQUEUE.
While here, further mimic the behavior of CAMIOQUEUE and copy the external buffer
into the internal buffer in the CCB and clear the pointer flag rather than using an
on-stack buffer.

Test Plan

• compiled only

[cem]

sys/cam/scsi/scsi_pass.c

2185–2189

Is dropping the periph lock problematic here? Could it disappear from under us?

2219

leaked?

[cem]

(There are other places in the code that drop the lock to do a sleeping operation, so that's probably at least not-a-regression.)

LGTM aside from the leak pointed out above.

[imp]

sys/cam/scsi/scsi_pass.c

2185–2189

No. We hold a ref to prevent that

[imp]

Module the leak that cem found . ...

[imp]

I mean modulo... stupid phone...

sys/cam/scsi/scsi_pass.c

2221

WTF? We return with the periph locked? that makes no sense to me... Not your issue, I'll hassle scottl...

[cem]

sys/cam/scsi/scsi_pass.c

2221

We enter periph locked, too. This function is locked entry, locked return. Whether that makes sense given we need to drop the lock some times, I don't know.

[jhb]

• Fix leak.

[imp]

sys/cam/scsi/scsi_pass.c

2184–2185

SCSI CDBs are at most 16 bytes, so page size is likely overkill. That's likely why it was originally on the stack. A better limit would be IOCDBLEN which is the ioctl limit or CAM_MAX_CDBLEN which is the in-kernel limit (they are the same in practice though). The sa driver just creates these on the stack... Maybe the even easier answer here is do just do the same.

2221

Ah, right... Not sure this really needs to be locked the whole time, but that's a different issue.

[jhb]

• Copy into cdb_bytes instead of the stack allocation.

[jhb]

sys/cam/scsi/scsi_pass.c

2184–2185

The only thing that gave me pause is that if the request is smaller than IOCDBLEN, you could just store it inline and not use CAM_CDB_POINTER at all. I assumed this meant that the only reason one would use CAM_CDB_POINTER was if the data was too large to fit into the cdb_io.cdb_bytes.

Hmm, it seems though that the existing IOCDBLEN check I see is in fact to copy via the pointer into cdb_bytes. Even more curious then why this doesn't do the same and copy into the cdb_bytes array rather than a stack allocation? I'd be happy with IOCDBLEN as the limit (see the code for handling this in the CAMIOQUEUE case)

[jhb]

Ping, are you happy with this version?

[imp]

LGTM, but this stuff is tricky so you may want to wait for it to look good to others too.

[jhb]

Added @scottl and @ken so they could look. We need a cam group on phab.

[scottl]

The point of all of this is that CDB's were predicted to grow larger than 16 bytes, and CAM was trying to optimize the common case of 6/10/12/16 byte CDB's without creating an API incompatibility for handling future growth. If a device came along that needed 32 byte CDB's, you could still use the stock CCB fields, but tack on the CDB as a separate allocation rather than have it be embedded in the CCB. So, while I agree with the original problem statement that the use of alloca() was unsafe, I disagree with the resolution.

[jhb]

In D24406#539548, @scottl wrote:

The point of all of this is that CDB's were predicted to grow larger than 16 bytes, and CAM was trying to optimize the common case of 6/10/12/16 byte CDB's without creating an API incompatibility for handling future growth. If a device came along that needed 32 byte CDB's, you could still use the stock CCB fields, but tack on the CDB as a separate allocation rather than have it be embedded in the CCB. So, while I agree with the original problem statement that the use of alloca() was unsafe, I disagree with the resolution.

So do you want it to go back to the malloc() version, and what upper-bound would you like on the allocation? (It needs an upper-bound, that is the real problem.) Also, the CAMIOQUEUE ioctl caps the length at 16 bytes still, so we should probably fix it to use the same upper-bound and to not copy into cdb_bytes but use a malloc'd pointer as well?

sys/cam/scsi/scsi_pass.c

1887

Length checks for CAMIOQUEUE.

[scottl]

In a pinch, I'd take it back to being a page size. I can see an argument that oversized CDBs and CAM_CDB_POINTER are underused to the point of creating unnecessary complication and risk to the code (as is evident from Alexander breaking it in r307205). No API was ever developed to make the feature easier to use, so it's been hit-and-miss on whether SIMs even support it. Maybe we eliminate it and deal with oversized CDBs via a new CCB type. We should also look into whether the embedded layout is creating unnecessary cache pollution in the CCB; maybe it makes sense to always have the CDB be a pointer to a scratch area at the end of the CCB that can grow as needed. Either way, we could bring everything into a conforming pattern that's easier and less risky.

sys/cam/scsi/scsi_pass.c

1887

This is wrong for the same reason as below.