From d3b58c006f4b6ad9380e1c7201209bf2d82657c5 Mon Sep 17 00:00:00 2001 From: Jim Harris Date: Tue, 6 Sep 2016 12:36:29 -0700 Subject: [PATCH] scsi: do not fill out more than allocation length for standard inquiry The translation code currently cheats a bit - it allocates a full 4KB buffer for any DATA_IN command that is not a READ, and then the different SCSI commands that fall into this category (INQUIRY, READ_CAPACITY, MODE_SENSE, etc.) can write as much data as they want without having to worry about a buffer overrun. Code higher up the stack makes sure we only send the correct amount of data back to the iSCSI initiator. This patch fixes this behavior for standard INQUIRY (EVPD = 0). Future patches will fix the behavior for other non-READ DATA_IN commands, at which point we can remove the 4KB allocation and only allocate the amount of data specified in the CDB. Signed-off-by: Jim Harris Change-Id: If5e4a10eeba9851e2d91cab71228d2fc2d5baad0 --- lib/scsi/scsi_bdev.c | 57 +++++++++++++++++++------- test/lib/scsi/scsi_bdev/scsi_bdev_ut.c | 48 ++++++++++++++++++++++ 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/lib/scsi/scsi_bdev.c b/lib/scsi/scsi_bdev.c index fac69aef8a..c74062e4c3 100644 --- a/lib/scsi/scsi_bdev.c +++ b/lib/scsi/scsi_bdev.c @@ -47,6 +47,9 @@ #define DEFAULT_DISK_ROTATION_RATE 7200 /* 7200 rpm */ #define DEFAULT_DISK_FORM_FACTOR 0x02 /* 3.5 inch */ +#define INQUIRY_OFFSET(field) offsetof(struct spdk_scsi_cdb_inquiry_data, field) + \ + sizeof(((struct spdk_scsi_cdb_inquiry_data *)0x0)->field) + static int spdk_hex2bin(char ch) { @@ -697,25 +700,50 @@ spdk_bdev_scsi_inquiry(struct spdk_bdev *bdev, struct spdk_scsi_task *task, /* PRODUCT REVISION LEVEL */ spdk_strcpy_pad(inqdata->product_rev, DEFAULT_DISK_REVISION, 4, ' '); - /* Vendor specific */ - memset(inqdata->vendor, 0x20, 20); + /* + * Standard inquiry data ends here. Only populate remaining fields if alloc_len + * indicates enough space to hold it. + */ + len = INQUIRY_OFFSET(product_rev) - 5; - /* CLOCKING(3-2) QAS(1) IUS(0) */ - inqdata->ius = 0; + if (alloc_len >= INQUIRY_OFFSET(vendor)) { + /* Vendor specific */ + memset(inqdata->vendor, 0x20, 20); + len += sizeof(inqdata->vendor); + } - /* Reserved */ - inqdata->reserved = 0; + if (alloc_len >= INQUIRY_OFFSET(ius)) { + /* CLOCKING(3-2) QAS(1) IUS(0) */ + inqdata->ius = 0; + len += sizeof(inqdata->ius); + } + + if (alloc_len >= INQUIRY_OFFSET(reserved)) { + /* Reserved */ + inqdata->reserved = 0; + len += sizeof(inqdata->reserved); + } /* VERSION DESCRIPTOR 1-8 */ - to_be16(inqdata->desc, 0x0960); - to_be16(&inqdata->desc[2], 0x0300); /* SPC-3 (no version claimed) */ - to_be16(&inqdata->desc[4], 0x320); /* SBC-2 (no version claimed) */ - to_be16(&inqdata->desc[6], 0x0040); /* SAM-2 (no version claimed) */ - /* 96 - 74 + 8 */ - /* Reserved[74-95] */ - memset(&inqdata->desc[8], 0, 30); + if (alloc_len >= INQUIRY_OFFSET(reserved) + 2) { + to_be16(&inqdata->desc[0], 0x0960); + len += 2; + } - len = alloc_len - hlen; + if (alloc_len >= INQUIRY_OFFSET(reserved) + 4) { + to_be16(&inqdata->desc[2], 0x0300); /* SPC-3 (no version claimed) */ + len += 2; + } + + if (alloc_len >= INQUIRY_OFFSET(reserved) + 6) { + to_be16(&inqdata->desc[4], 0x320); /* SBC-2 (no version claimed) */ + len += 2; + } + + if (alloc_len >= INQUIRY_OFFSET(reserved) + 8) { + to_be16(&inqdata->desc[6], 0x0040); /* SAM-2 (no version claimed) */ + len += 2; + } /* ADDITIONAL LENGTH */ inqdata->add_len = len; @@ -724,6 +752,7 @@ spdk_bdev_scsi_inquiry(struct spdk_bdev *bdev, struct spdk_scsi_task *task, return hlen + len; inq_error: + task->data_transferred = 0; spdk_scsi_task_set_check_condition(task, SPDK_SCSI_SENSE_NO_SENSE, 0x0, 0x0); diff --git a/test/lib/scsi/scsi_bdev/scsi_bdev_ut.c b/test/lib/scsi/scsi_bdev/scsi_bdev_ut.c index e00e176658..3b69abccc3 100644 --- a/test/lib/scsi/scsi_bdev/scsi_bdev_ut.c +++ b/test/lib/scsi/scsi_bdev/scsi_bdev_ut.c @@ -416,6 +416,53 @@ inquiry_standard_test(void) CU_ASSERT_EQUAL(rc, 0); } +static void +_inquiry_overflow_test(uint8_t alloc_len) +{ + struct spdk_bdev bdev; + struct spdk_scsi_task task; + struct spdk_scsi_lun lun; + struct spdk_scsi_dev dev; + struct spdk_bdev_fn_table fn_table; + uint8_t cdb[6]; + /* expects a 4K internal data buffer */ + char data[256], data_compare[256]; + int rc; + + bdev.fn_table = &fn_table; + + cdb[0] = 0x12; + cdb[1] = 0x00; // EVPD = 0 + cdb[2] = 0x00; // PageCode zero - requesting standard inquiry + cdb[3] = 0x00; + cdb[4] = alloc_len; // Indicate data size used by conformance test + cdb[5] = 0x00; + task.cdb = cdb; + + snprintf(&dev.name[0], sizeof(dev.name), "spdk_iscsi_translation_test"); + lun.dev = &dev; + task.lun = &lun; + + memset(data, 0, sizeof(data)); + memset(data_compare, 0, sizeof(data_compare)); + task.rbuf = data; + + rc = spdk_bdev_scsi_execute(&bdev, &task); + CU_ASSERT_EQUAL(rc, 0); + CU_ASSERT_EQUAL(memcmp(data + alloc_len, data_compare + alloc_len, sizeof(data) - alloc_len), 0); + CU_ASSERT(task.data_transferred <= alloc_len); +} + +static void +inquiry_overflow_test(void) +{ + int i; + + for (i = 0; i < 256; i++) { + _inquiry_overflow_test(i); + } +} + int main(int argc, char **argv) { @@ -439,6 +486,7 @@ main(int argc, char **argv) || CU_add_test(suite, "mode sense 10 test", mode_sense_10_test) == NULL || CU_add_test(suite, "inquiry evpd test", inquiry_evpd_test) == NULL || CU_add_test(suite, "inquiry standard test", inquiry_standard_test) == NULL + || CU_add_test(suite, "inquiry overflow test", inquiry_overflow_test) == NULL ) { CU_cleanup_registry(); return CU_get_error();