| Message ID | 20220617030440.116427-2-michael.christie@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | target unmap/writespace fixes and enhancements | expand |
On Thu, Jun 16, 2022 at 10:04:36PM -0500, Mike Christie wrote: > If the WRITE_SAME NDOB bit is set then there is not going to be a > buffer. LIO core will then complain: > > TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB > Length: 512 for SAM Opcode: 0x93 > > This fixes the issue by detecting when the bit is set and adjusting the > size. The patch looks good and useful, but right the taret code doesn't even support MI_REPORT_SUPPORTED_OPERATION_CODES to report support for NDOB, so who ends up using it?
On 6/19/22 1:22 AM, Christoph Hellwig wrote: > On Thu, Jun 16, 2022 at 10:04:36PM -0500, Mike Christie wrote: >> If the WRITE_SAME NDOB bit is set then there is not going to be a >> buffer. LIO core will then complain: >> >> TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB >> Length: 512 for SAM Opcode: 0x93 >> >> This fixes the issue by detecting when the bit is set and adjusting the >> size. > > > The patch looks good and useful, but right the taret code doesn't even > support MI_REPORT_SUPPORTED_OPERATION_CODES to report support for NDOB, > so who ends up using it? sg_write_same allows it. We found the bug because some user just decided to do: sg_write_same ... -nbod .. /dev/sdb and it crashed the box. I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it. I don't need support for the feature. I just want to fix the crash. I prefer just returning failure since nothing ever has ever used it if other people prefer that as well.
On Sun, Jun 19, 2022 at 11:25:33AM -0500, michael.christie@oracle.com wrote: > sg_write_same allows it. We found the bug because some user just decided > to do: > > sg_write_same ... -nbod .. /dev/sdb > > and it crashed the box. Oh. > I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it. > I don't need support for the feature. I just want to fix the crash. > I prefer just returning failure since nothing ever has ever used it if > other people prefer that as well. I think the feature is generally useful, and I know Martin had patches to use it in Linux. But I think a minimal fix for the remotely exploitable crash has the highest priority. Where does it crash? Maybe we just need a better sanity check somewhere if a command claims to transfer data but has not payload?
On 6/20/22 1:45 AM, Christoph Hellwig wrote: > On Sun, Jun 19, 2022 at 11:25:33AM -0500, michael.christie@oracle.com wrote: >> sg_write_same allows it. We found the bug because some user just decided >> to do: >> >> sg_write_same ... -nbod .. /dev/sdb >> >> and it crashed the box. > > Oh. > >> I didn't know about the MI_REPORT_SUPPORTED_OPERATION_CODES part of it. >> I don't need support for the feature. I just want to fix the crash. >> I prefer just returning failure since nothing ever has ever used it if >> other people prefer that as well. > > I think the feature is generally useful, and I know Martin had patches > to use it in Linux. But I think a minimal fix for the remotely I'll work with Martin to find if there is an oracle user to test and on a longer term feature addition. > exploitable crash has the highest priority. Where does it crash? It crashes when we first access the sg in file and iblock's execute_write_same functions. > Maybe we just need a better sanity check somewhere if a command > claims to transfer data but has not payload? I'll look into it and send a patch.
diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c index ca1b2312d6e7..6d98b016a942 100644 --- a/drivers/target/target_core_sbc.c +++ b/drivers/target/target_core_sbc.c @@ -976,7 +976,11 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) return TCM_INVALID_CDB_FIELD; } - size = sbc_get_size(cmd, 1); + /* If NDOB is set there will not be a Data-Out buffer */ + if (cdb[1] & 0x01) + size = 0; + else + size = sbc_get_size(cmd, 1); cmd->t_task_lba = get_unaligned_be64(&cdb[12]); ret = sbc_setup_write_same(cmd, cdb[10], ops); @@ -1075,7 +1079,11 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops) return TCM_INVALID_CDB_FIELD; } - size = sbc_get_size(cmd, 1); + /* If NDOB is set there will not be a Data-Out buffer */ + if (cdb[1] & 0x01) + size = 0; + else + size = sbc_get_size(cmd, 1); cmd->t_task_lba = get_unaligned_be64(&cdb[2]); ret = sbc_setup_write_same(cmd, cdb[1], ops);
If the WRITE_SAME NDOB bit is set then there is not going to be a buffer. LIO core will then complain: TARGET_CORE[iSCSI]: Expected Transfer Length: 0 does not match SCSI CDB Length: 512 for SAM Opcode: 0x93 This fixes the issue by detecting when the bit is set and adjusting the size. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/target/target_core_sbc.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)