[PATCH 0/7] usb: storage: some llm-found patches

[PATCH 0/7] usb: storage: some llm-found patches

[Greg Kroah-Hartman]

Here are some small llm-found patches for the odd usb-storage drivers
that were sent to me.

None of them have been tested, but they seem semi-sane so I thought I'd
submit them for others to see and Sashiko to give a second pass at.

Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Josef Bacik (7):
      usb: storage: ene_ub6250: fail MS init on card errors
      usb: storage: ene_ub6250: report boot block errors
      usb: storage: ene_ub6250: fix write to unmapped MS block
      usb: storage: ene_ub6250: bound the disabled block walk
      usb: storage: sddr09: bound the unused PBA search
      usb: storage: shuttle_usbat: fix READ CD divide by zero
      usb: storage: shuttle_usbat: bound the READ chunk size

 drivers/usb/storage/ene_ub6250.c    | 18 +++++++++---------
 drivers/usb/storage/sddr09.c        |  2 +-
 drivers/usb/storage/shuttle_usbat.c |  9 +++++++--
 3 files changed, 17 insertions(+), 12 deletions(-)
---
base-commit: d135ab453a7400a34930bdda1ef4bdc8acfac80e
change-id: 20261027-llm-usb-storage-6d3bfec890ee

Best regards,
--  
Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 1/7] usb: storage: ene_ub6250: fail MS init on card errors

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

Inserting a Memory Stick that has no usable boot block, for example a
blank or damaged card, into an ENE UB6250 reader oopses the kernel with
a divide error on the first READ(10).

ene_ms_init() ignores the return value of ms_card_init().  When
ms_card_init() finds no boot block it returns MS_STATUS_ERROR after
ms_lib_free_allocatedarea() has cleared the card geometry and freed the
logical maps, but MS_Status keeps the MS_Ready bit the reader reported,
so ene_transport() goes on to dispatch commands to ms_scsi_irp().  The
first READ(10) from the partition scan then reaches ms_scsi_read():

    logblk  = (u16)(bn / info->MS_Lib.PagesPerBlock);

with PagesPerBlock == 0.  With a non-zero PagesPerBlock the same path
would dereference the freed (NULL) Log2PhyMap instead.

The bug was found by code inspection and has not been reproduced.

Fix by clearing MS_Ready and returning USB_STOR_TRANSPORT_ERROR from
ene_ms_init() when ms_card_init() fails, as is already done when the
reader reports that the card is not ready.

Fixes: 33842cedfc33 ("Staging: Merge ENE UB6250 MS card codes from keucr to drivers/usb/storage/ene_ub6250.c")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/ene_ub6250.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index a9358e04ab4b..49276ac79402 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -2110,8 +2110,9 @@ static int ene_ms_init(struct us_data *us)
 			MSP_BlockSize      = (bbuf[6] << 8) | bbuf[7];
 			MSP_UserAreaBlocks = (bbuf[10] << 8) | bbuf[11];
 			info->MSP_TotalBlock = MSP_BlockSize * MSP_UserAreaBlocks;
-		} else {
-			ms_card_init(us); /* Card is MS (to ms.c)*/
+		} else if (ms_card_init(us)) { /* Card is MS (to ms.c)*/
+			info->MS_Status &= ~MS_Ready;
+			return USB_STOR_TRANSPORT_ERROR;
 		}
 		usb_stor_dbg(us, "MS Init Code OK !!\n");
 	} else {

-- 
2.56.0

[PATCH 2/7] usb: storage: ene_ub6250: report boot block errors

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

A Memory Stick whose boot block is rejected by the ENE UB6250 driver,
for instance a hybrid card or a card with an unexpected class, page size
or IDI configuration, crashes the kernel with a NULL pointer
write when it is inserted.

ms_lib_process_bootblock() unconditionally sets result to 0 just before
returning, so every failure is reported as success.  On those failures
the logical maps were either never allocated, or were freed again by
ms_lib_free_logicalmap() at its exit label, so ms_card_init() then
writes through a NULL Phy2LogMap:

    info->MS_Lib.Phy2LogMap[btBlk1st] = MS_LB_BOOT_BLOCK;

It also means the second boot block is never tried when the first one is
bad.

The bug was found by code inspection and has not been reproduced.

Fix by returning the real result from ms_lib_process_bootblock(), so
that ms_card_init() falls back to the second boot block and fails when
neither one is usable.

Fixes: 33842cedfc33 ("Staging: Merge ENE UB6250 MS card codes from keucr to drivers/usb/storage/ene_ub6250.c")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/ene_ub6250.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index 49276ac79402..a5ef1dcf0bc7 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -1064,7 +1064,6 @@ static int ms_lib_process_bootblock(struct us_data *us, u16 PhyBlock, u8 *PageDa
 
 	kfree(PageBuffer);
 
-	result = 0;
 	return result;
 }
 

-- 
2.56.0

[PATCH 3/7] usb: storage: ene_ub6250: fix write to unmapped MS block

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

Writing to a logical block of a Memory Stick that has no physical block
assigned yet makes the ENE UB6250 driver write two bytes far past the
end of its Phy2LogMap allocation.

ms_scsi_write() looks up the current physical block of each logical
block with ms_libconv_to_physical().  For a logical block that no
physical block claimed during the scan, Log2PhyMap holds MS_LB_NOT_USED
(0xffff), and for a logical block beyond NumberOfLogBlock the macro
returns MS_STATUS_ERROR (0xfffe).  After copying the data to the new
block, the old one is marked as erased without any check:

    info->MS_Lib.Phy2LogMap[oldphy] = MS_LB_NOT_USED_ERASED;

which, for those values, stores 0xfff5 about 128KiB past the start of
an array that holds only NumberOfPhyBlock entries.  Unassigned logical
blocks are normal on a card that has not been completely written, and
the card contents are under the control of whoever supplies the card
or the reader.

The bug was found by code inspection and has not been reproduced.

Fix by only marking the old physical block as erased when it is a valid
physical block number.

Fixes: 33842cedfc33 ("Staging: Merge ENE UB6250 MS card codes from keucr to drivers/usb/storage/ene_ub6250.c")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/ene_ub6250.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index a5ef1dcf0bc7..bce593730187 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -1805,7 +1805,8 @@ static int ms_scsi_write(struct us_data *us, struct scsi_cmnd *srb)
 				goto exit;
 			}
 
-			info->MS_Lib.Phy2LogMap[oldphy] = MS_LB_NOT_USED_ERASED;
+			if (oldphy < info->MS_Lib.NumberOfPhyBlock)
+				info->MS_Lib.Phy2LogMap[oldphy] = MS_LB_NOT_USED_ERASED;
 			ms_lib_force_setlogical_pair(us, PhyBlockAddr, newphy);
 
 			blen -= len;

-- 
2.56.0

[PATCH 4/7] usb: storage: ene_ub6250: bound the disabled block walk

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

When two physical blocks of a Memory Stick both claim logical block 0,
which happens after an interrupted update of that block or on a crafted
card, the ENE UB6250 driver can spin forever in the usb-storage thread
while reading through 64KiB past a 512-byte buffer, over and over.

ms_lib_check_disableblock() reads page 1 of the boot block, which holds
the disabled block table: big-endian 16-bit block numbers terminated by
0xffff, the same table ms_lib_process_bootblock() parses with
be16_to_cpu() on 16-bit entries.  The walk, however, indexes the buffer
as unsigned char and has no upper bound:

    blk = be16_to_cpu(PageBuf[index]);
    if (blk == MS_LB_NOT_USED)
            break;

A byte converted this way can never equal 0xffff, so the loop only ends
if some byte happens to match Log2PhyMap[0].  Otherwise the u16 index
runs through the 64KiB following the kmalloc()ed page and wraps around,
forever.

The bug was found by code inspection and has not been reproduced.

Fix by walking the table as 16-bit big-endian entries and stopping at
the end of the page.

Fixes: 33842cedfc33 ("Staging: Merge ENE UB6250 MS card codes from keucr to drivers/usb/storage/ene_ub6250.c")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/ene_ub6250.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/storage/ene_ub6250.c b/drivers/usb/storage/ene_ub6250.c
index bce593730187..e282ca9b5e16 100644
--- a/drivers/usb/storage/ene_ub6250.c
+++ b/drivers/usb/storage/ene_ub6250.c
@@ -1210,9 +1210,9 @@ static int ms_read_eraseblock(struct us_data *us, u32 PhyBlockAddr)
 
 static int ms_lib_check_disableblock(struct us_data *us, u16 PhyBlock)
 {
-	unsigned char *PageBuf = NULL;
+	__be16 *PageBuf = NULL;
 	u16 result = MS_STATUS_SUCCESS;
-	u16 blk, index = 0;
+	u16 blk, index;
 	struct ms_lib_type_extdat extdat;
 	struct ene_ub6250_info *info = (struct ene_ub6250_info *) us->extra;
 
@@ -1223,7 +1223,7 @@ static int ms_lib_check_disableblock(struct us_data *us, u16 PhyBlock)
 	}
 
 	ms_read_readpage(us, PhyBlock, 1, (u32 *)PageBuf, &extdat);
-	do {
+	for (index = 0; index < MS_BYTES_PER_PAGE / sizeof(*PageBuf); index++) {
 		blk = be16_to_cpu(PageBuf[index]);
 		if (blk == MS_LB_NOT_USED)
 			break;
@@ -1231,8 +1231,7 @@ static int ms_lib_check_disableblock(struct us_data *us, u16 PhyBlock)
 			result = MS_ERROR_FLASH_READ;
 			break;
 		}
-		index++;
-	} while (1);
+	}
 
 exit:
 	kfree(PageBuf);

-- 
2.56.0

[PATCH 5/7] usb: storage: sddr09: bound the unused PBA search

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

sddr09_find_unused_pba() can read past the end of pba_to_lba[] and hand
out a physical block that does not exist on the card, which is then
recorded in pba_to_lba[] past its end and written to.

The search resumes from lastpba, a function-local static shared by all
SDDR-09 readers and kept across card changes.  The first loop is bounded
by end, the number of blocks in the zone, but the wrap-around loop only
by lastpba:

    for (i = 0; i <= lastpba; i++) {
            if (info->pba_to_lba[zonestart+i] == UNDEF) {

After a write to a card with 1024 blocks per zone has left lastpba at,
say, 700, writing to a 1MB card (256 blocks) whose blocks are all in use
walks pba_to_lba[] from index 256 to 700.  If any of those out-of-bounds
words reads as UNDEF, sddr09_write_lba() stores into pba_to_lba[pba]
and goes on to program that bogus block.

The bug was found by code inspection and has not been reproduced.

Fix by also stopping the wrap-around loop at the end of the zone.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/sddr09.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/sddr09.c b/drivers/usb/storage/sddr09.c
index 313313508bde..44197a3a545b 100644
--- a/drivers/usb/storage/sddr09.c
+++ b/drivers/usb/storage/sddr09.c
@@ -838,7 +838,7 @@ sddr09_find_unused_pba(struct sddr09_card_info *info, unsigned int lba) {
 			return zonestart+i;
 		}
 	}
-	for (i = 0; i <= lastpba; i++) {
+	for (i = 0; i <= lastpba && i < end; i++) {
 		if (info->pba_to_lba[zonestart+i] == UNDEF) {
 			lastpba = i;
 			return zonestart+i;

-- 
2.56.0

[PATCH 6/7] usb: storage: shuttle_usbat: fix READ CD divide by zero

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

A READ CD command of 64KiB or more with a zero transfer length, which
any user who can open the CD device node may send with SG_IO, oopses
the kernel with a divide error on an HP 8200e CD-RW behind a USBAT
bridge.

usbat_hp8200e_handle_read10() splits large reads into chunks of whole
sectors and, for READ CD, derives the sector size from the transfer
length taken from the CDB:

    srb->transfersize = scsi_bufflen(srb)/len;

Nothing guarantees that this length is non-zero.  The code right after
it already falls back to a 2048-byte guess when transfersize is 0.

The bug was found by code inspection and has not been reproduced.

Fix by only dividing by a non-zero transfer length and otherwise
leaving transfersize at 0 so that the existing fallback applies.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/shuttle_usbat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/storage/shuttle_usbat.c b/drivers/usb/storage/shuttle_usbat.c
index 7e5424268c73..a1b64b10bbe0 100644
--- a/drivers/usb/storage/shuttle_usbat.c
+++ b/drivers/usb/storage/shuttle_usbat.c
@@ -1325,7 +1325,7 @@ static int usbat_hp8200e_handle_read10(struct us_data *us,
 		len <<= 16;
 		len |= data[7+7];
 		usb_stor_dbg(us, "GPCMD_READ_CD: len %d\n", len);
-		srb->transfersize = scsi_bufflen(srb)/len;
+		srb->transfersize = len ? scsi_bufflen(srb) / len : 0;
 	}
 
 	if (!srb->transfersize)  {

-- 
2.56.0

[PATCH 7/7] usb: storage: shuttle_usbat: bound the READ chunk size

[Greg Kroah-Hartman]

From: Josef Bacik <josef@toxicpanda.com>

An SG_IO READ(10) of 64KiB or more to an HP 8200e CD-RW behind a USBAT
bridge never makes progress: the usb-storage thread keeps issuing
zero-length reads until the SCSI command times out and is aborted.

usbat_hp8200e_handle_read10() splits reads of 64KiB or more into chunks
of whole sectors, using srb->transfersize as the sector size:

    len = (65535/srb->transfersize) * srb->transfersize;

For passthrough requests scsi_setup_scsi_cmnd() sets transfersize to
the whole request size, and the READ CD path can likewise compute a
value above 65535, so len becomes 0.  The loop then sends commands with
a zero length and adds 0 to the transferred count on every pass.

The bug was found by code inspection and has not been reproduced.

Fix by falling back to the existing 2048-byte sector size guess when
transfersize is larger than the maximum read size, so that every chunk
holds at least one sector.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/storage/shuttle_usbat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/storage/shuttle_usbat.c b/drivers/usb/storage/shuttle_usbat.c
index a1b64b10bbe0..080799029643 100644
--- a/drivers/usb/storage/shuttle_usbat.c
+++ b/drivers/usb/storage/shuttle_usbat.c
@@ -1328,7 +1328,12 @@ static int usbat_hp8200e_handle_read10(struct us_data *us,
 		srb->transfersize = len ? scsi_bufflen(srb) / len : 0;
 	}
 
-	if (!srb->transfersize)  {
+	/*
+	 * Passthrough requests set transfersize to the whole request size,
+	 * which can exceed the 64k-1 maximum read and leave no whole sector
+	 * to read per chunk.
+	 */
+	if (!srb->transfersize || srb->transfersize > 65535) {
 		srb->transfersize = 2048; /* A guess */
 		usb_stor_dbg(us, "transfersize 0, forcing %d\n",
 			     srb->transfersize);

-- 
2.56.0