From 14faa944b6fa4c77a6f386806c33ce2c3c77b3a4 Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Wed, 18 Sep 2013 21:27:24 +0900 Subject: [SCSI] scsi_debug: fix buffer overrun when DIF/DIX is enabled and virtual_gb > 0 If the module parameter virtual_gb is greater than 0, the READ command may request the blocks which exceed actual ramdisk storage (fake_storep). prot_verify_read() should treat those blocks as wrap around the end of fake_storep. But it actually causes fake_storep and dif_storep buffer overruns. This fixes these buffer overruns. In order to simplify the fix, this also introduces fake_store() and dif_store() which return corresponding wrap around addresses. Signed-off-by: Akinobu Mita Acked-by: Martin K. Petersen Signed-off-by: James Bottomley --- drivers/scsi/scsi_debug.c | 48 +++++++++++++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 18 deletions(-) (limited to 'drivers/scsi/scsi_debug.c') diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 01c0ffa31276..f640b6b380bd 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -293,6 +293,20 @@ static unsigned char ctrl_m_pg[] = {0xa, 10, 2, 0, 0, 0, 0, 0, static unsigned char iec_m_pg[] = {0x1c, 0xa, 0x08, 0, 0, 0, 0, 0, 0, 0, 0x0, 0x0}; +static void *fake_store(unsigned long long lba) +{ + lba = do_div(lba, sdebug_store_sectors); + + return fake_storep + lba * scsi_debug_sector_size; +} + +static struct sd_dif_tuple *dif_store(sector_t sector) +{ + sector = do_div(sector, sdebug_store_sectors); + + return dif_storep + sector; +} + static int sdebug_add_adapter(void); static void sdebug_remove_adapter(void); @@ -1782,24 +1796,19 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, struct scatterlist *psgl; struct sd_dif_tuple *sdt; sector_t sector; - sector_t tmp_sec = start_sec; void *paddr; + const void *dif_store_end = dif_storep + sdebug_store_sectors; - start_sec = do_div(tmp_sec, sdebug_store_sectors); - - sdt = dif_storep + start_sec; - - for (i = 0 ; i < sectors ; i++) { + for (i = 0; i < sectors; i++) { int ret; - if (sdt[i].app_tag == 0xffff) - continue; - sector = start_sec + i; + sdt = dif_store(sector); - ret = dif_verify(&sdt[i], - fake_storep + sector * scsi_debug_sector_size, - sector, ei_lba); + if (sdt->app_tag == 0xffff) + continue; + + ret = dif_verify(sdt, fake_store(sector), sector, ei_lba); if (ret) { dif_errors++; return ret; @@ -1814,16 +1823,19 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, scsi_for_each_prot_sg(SCpnt, psgl, scsi_prot_sg_count(SCpnt), i) { int len = min(psgl->length, resid); + void *start = dif_store(sector); + int rest = 0; + + if (dif_store_end < start + len) + rest = start + len - dif_store_end; paddr = kmap_atomic(sg_page(psgl)) + psgl->offset; - memcpy(paddr, dif_storep + sector, len); + memcpy(paddr, start, len - rest); + + if (rest) + memcpy(paddr + len - rest, dif_storep, rest); sector += len / sizeof(*dif_storep); - if (sector >= sdebug_store_sectors) { - /* Force wrap */ - tmp_sec = sector; - sector = do_div(tmp_sec, sdebug_store_sectors); - } resid -= len; kunmap_atomic(paddr); } -- cgit v1.2.3 From bb8c063c6afcd930b8da944927144f2982609638 Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Wed, 18 Sep 2013 21:27:25 +0900 Subject: [SCSI] scsi_debug: factor out copying PI from dif_storep to prot_sglist If data integrity support is enabled, prot_verify_read() is called in response to READ commands and it verifies protection info from dif_storep by comparing against fake_storep, and copies protection info to prot_sglist. This factors out the portion of copying protection info into a separate function. It will also be reused in the next change after supporting the opposite direction (copying prot_sglist to dif_storep). Signed-off-by: Akinobu Mita Acked-by: Martin K. Petersen Signed-off-by: James Bottomley --- drivers/scsi/scsi_debug.c | 52 ++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 23 deletions(-) (limited to 'drivers/scsi/scsi_debug.c') diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index f640b6b380bd..99e74d75cf08 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1789,37 +1789,16 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data, return 0; } -static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, - unsigned int sectors, u32 ei_lba) +static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector, + unsigned int sectors) { unsigned int i, resid; struct scatterlist *psgl; - struct sd_dif_tuple *sdt; - sector_t sector; void *paddr; const void *dif_store_end = dif_storep + sdebug_store_sectors; - for (i = 0; i < sectors; i++) { - int ret; - - sector = start_sec + i; - sdt = dif_store(sector); - - if (sdt->app_tag == 0xffff) - continue; - - ret = dif_verify(sdt, fake_store(sector), sector, ei_lba); - if (ret) { - dif_errors++; - return ret; - } - - ei_lba++; - } - /* Bytes of protection data to copy into sgl */ resid = sectors * sizeof(*dif_storep); - sector = start_sec; scsi_for_each_prot_sg(SCpnt, psgl, scsi_prot_sg_count(SCpnt), i) { int len = min(psgl->length, resid); @@ -1839,7 +1818,34 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, resid -= len; kunmap_atomic(paddr); } +} + +static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, + unsigned int sectors, u32 ei_lba) +{ + unsigned int i; + struct sd_dif_tuple *sdt; + sector_t sector; + + for (i = 0; i < sectors; i++) { + int ret; + + sector = start_sec + i; + sdt = dif_store(sector); + + if (sdt->app_tag == 0xffff) + continue; + + ret = dif_verify(sdt, fake_store(sector), sector, ei_lba); + if (ret) { + dif_errors++; + return ret; + } + + ei_lba++; + } + dif_copy_prot(SCpnt, start_sec, sectors); dix_reads++; return 0; -- cgit v1.2.3 From 65f72f2a2fe89f072d6a88e5cd69a64270b9c436 Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Wed, 18 Sep 2013 21:27:26 +0900 Subject: [SCSI] scsi_debug: avoid partial copying PI from prot_sglist to dif_storep If data integrity support is enabled, prot_verify_write() is called in response to WRITE commands and it verifies protection info from prot_sglist by comparing against data sglist, and copies protection info to dif_storep. When multiple blocks are transfered by a WRITE command, it verifies and copies these blocks one by one. So if it fails to verify protection info in the middle of blocks, the actual data transfer to fake_storep isn't proceeded at all although protection info for some blocks are already copied to dif_storep. Therefore, it breaks the data integrity between fake_storep and dif_storep. This fixes it by ensuring that copying protection info to dif_storep is done after all blocks are successfully verified. Reusing dif_copy_prot() with supporting the opposite direction simplifies this fix. Signed-off-by: Akinobu Mita Acked-by: Martin K. Petersen Signed-off-by: James Bottomley --- drivers/scsi/scsi_debug.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-) (limited to 'drivers/scsi/scsi_debug.c') diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 99e74d75cf08..43369e9071f7 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1790,7 +1790,7 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data, } static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector, - unsigned int sectors) + unsigned int sectors, bool read) { unsigned int i, resid; struct scatterlist *psgl; @@ -1809,10 +1809,18 @@ static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector, rest = start + len - dif_store_end; paddr = kmap_atomic(sg_page(psgl)) + psgl->offset; - memcpy(paddr, start, len - rest); - if (rest) - memcpy(paddr + len - rest, dif_storep, rest); + if (read) + memcpy(paddr, start, len - rest); + else + memcpy(start, paddr, len - rest); + + if (rest) { + if (read) + memcpy(paddr + len - rest, dif_storep, rest); + else + memcpy(dif_storep, paddr + len - rest, rest); + } sector += len / sizeof(*dif_storep); resid -= len; @@ -1845,7 +1853,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, ei_lba++; } - dif_copy_prot(SCpnt, start_sec, sectors); + dif_copy_prot(SCpnt, start_sec, sectors, true); dix_reads++; return 0; @@ -1928,15 +1936,12 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec, { int i, j, ret; struct sd_dif_tuple *sdt; - struct scatterlist *dsgl = scsi_sglist(SCpnt); + struct scatterlist *dsgl; struct scatterlist *psgl = scsi_prot_sglist(SCpnt); void *daddr, *paddr; - sector_t tmp_sec = start_sec; - sector_t sector; + sector_t sector = start_sec; int ppage_offset; - sector = do_div(tmp_sec, sdebug_store_sectors); - BUG_ON(scsi_sg_count(SCpnt) == 0); BUG_ON(scsi_prot_sg_count(SCpnt) == 0); @@ -1964,25 +1969,13 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec, sdt = paddr + ppage_offset; - ret = dif_verify(sdt, daddr + j, start_sec, ei_lba); + ret = dif_verify(sdt, daddr + j, sector, ei_lba); if (ret) { dump_sector(daddr + j, scsi_debug_sector_size); goto out; } - /* Would be great to copy this in bigger - * chunks. However, for the sake of - * correctness we need to verify each sector - * before writing it to "stable" storage - */ - memcpy(dif_storep + sector, sdt, sizeof(*sdt)); - sector++; - - if (sector == sdebug_store_sectors) - sector = 0; /* Force wrap */ - - start_sec++; ei_lba++; ppage_offset += sizeof(struct sd_dif_tuple); } @@ -1991,6 +1984,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec, kunmap_atomic(daddr); } + dif_copy_prot(SCpnt, start_sec, sectors, false); dix_writes++; return 0; -- cgit v1.2.3 From 68aee7ba66d390abf48c13791a84f6bce29d6f19 Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Wed, 18 Sep 2013 21:27:27 +0900 Subject: [SCSI] scsi_debug: fix invalid value check for guard module parameter In the module initialization, invalid value for guard module parameter is detected by the following check: if (scsi_debug_guard > 1) { printk(KERN_ERR "scsi_debug_init: guard must be 0 or 1\n"); return -EINVAL; } But this check isn't enough, because the type of scsi_debug_guard is 'int' and scsi_debug_guard could be a negative value. This fixes it by changing the type of scsi_debug_guard to 'unsigned int' instead of adding extra check for a negative value. Reported-by: Joe Perches Signed-off-by: Akinobu Mita Acked-by: Martin K. Petersen Signed-off-by: James Bottomley --- drivers/scsi/scsi_debug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'drivers/scsi/scsi_debug.c') diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 43369e9071f7..a21322d6da61 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -169,7 +169,7 @@ static int scsi_debug_dix = DEF_DIX; static int scsi_debug_dsense = DEF_D_SENSE; static int scsi_debug_every_nth = DEF_EVERY_NTH; static int scsi_debug_fake_rw = DEF_FAKE_RW; -static int scsi_debug_guard = DEF_GUARD; +static unsigned int scsi_debug_guard = DEF_GUARD; static int scsi_debug_lowest_aligned = DEF_LOWEST_ALIGNED; static int scsi_debug_max_luns = DEF_MAX_LUNS; static int scsi_debug_max_queue = SCSI_DEBUG_CANQUEUE; @@ -2754,7 +2754,7 @@ module_param_named(dix, scsi_debug_dix, int, S_IRUGO); module_param_named(dsense, scsi_debug_dsense, int, S_IRUGO | S_IWUSR); module_param_named(every_nth, scsi_debug_every_nth, int, S_IRUGO | S_IWUSR); module_param_named(fake_rw, scsi_debug_fake_rw, int, S_IRUGO | S_IWUSR); -module_param_named(guard, scsi_debug_guard, int, S_IRUGO); +module_param_named(guard, scsi_debug_guard, uint, S_IRUGO); module_param_named(lbpu, scsi_debug_lbpu, int, S_IRUGO); module_param_named(lbpws, scsi_debug_lbpws, int, S_IRUGO); module_param_named(lbpws10, scsi_debug_lbpws10, int, S_IRUGO); @@ -3184,7 +3184,7 @@ DRIVER_ATTR(dif, S_IRUGO, sdebug_dif_show, NULL); static ssize_t sdebug_guard_show(struct device_driver *ddp, char *buf) { - return scnprintf(buf, PAGE_SIZE, "%d\n", scsi_debug_guard); + return scnprintf(buf, PAGE_SIZE, "%u\n", scsi_debug_guard); } DRIVER_ATTR(guard, S_IRUGO, sdebug_guard_show, NULL); -- cgit v1.2.3 From 51d648af5892219cbe97305efb300d3e56746591 Mon Sep 17 00:00:00 2001 From: Akinobu Mita Date: Wed, 18 Sep 2013 21:27:28 +0900 Subject: [SCSI] scsi_debug: fix sparse warnings related to data integrity field Each member in data integrity field tuple is big-endian. But the endianness of the values being compared with these members are not annotated. So this fixes these sparse warnings. Reported-by: kbuild test robot Signed-off-by: Akinobu Mita Acked-by: Martin K. Petersen Signed-off-by: James Bottomley --- drivers/scsi/scsi_debug.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) (limited to 'drivers/scsi/scsi_debug.c') diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index a21322d6da61..80b8b10edf41 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -1745,25 +1745,22 @@ static int do_device_access(struct scsi_cmnd *scmd, return ret; } -static u16 dif_compute_csum(const void *buf, int len) +static __be16 dif_compute_csum(const void *buf, int len) { - u16 csum; + __be16 csum; - switch (scsi_debug_guard) { - case 1: - csum = ip_compute_csum(buf, len); - break; - case 0: + if (scsi_debug_guard) + csum = (__force __be16)ip_compute_csum(buf, len); + else csum = cpu_to_be16(crc_t10dif(buf, len)); - break; - } + return csum; } static int dif_verify(struct sd_dif_tuple *sdt, const void *data, sector_t sector, u32 ei_lba) { - u16 csum = dif_compute_csum(data, scsi_debug_sector_size); + __be16 csum = dif_compute_csum(data, scsi_debug_sector_size); if (sdt->guard_tag != csum) { pr_err("%s: GUARD check failed on sector %lu rcvd 0x%04x, data 0x%04x\n", @@ -1841,7 +1838,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, sector = start_sec + i; sdt = dif_store(sector); - if (sdt->app_tag == 0xffff) + if (sdt->app_tag == cpu_to_be16(0xffff)) continue; ret = dif_verify(sdt, fake_store(sector), sector, ei_lba); -- cgit v1.2.3