[PATCH v2] scsi: core: reject short REPORT LUNS responses

[shao.mingyin]

scsi_report_lun_scan() reads the LUN list length from a buffer that was
allocated with kmalloc() and that the device may not have filled in
completely.  scsi_execute_cmd() zeroes the part of the buffer that the
host reported as not transferred, but only when the host reports a
residual at all, and it cannot tell whether a transfer that stopped in
the middle of the eight byte LUN list header left a usable length
field.

A host that completes the command without reporting a residual leaves
the kmalloc()ed contents in place, so stale heap data is parsed as a
length.  A host that reports the residual but stopped inside the header
leaves the bytes it did transfer to be read as that length, and neither
the retry allocation nor the number of descriptors to scan is then
bounded by what was actually transferred.

Allocate the buffer with kzalloc() so that a host which reports no
residual and transfers nothing cannot hand back stale heap data, and
ask for the residual so that a transfer which stopped inside the header
is rejected before its length is trusted.

The check requires the full eight byte LUN list header (the four byte
length plus the four reserved bytes), not just the length field, so a
transfer that stopped anywhere inside the header is rejected.  It also
rejects a length that cannot hold the header at all: the retry path
derives the length from the device-reported value with an addition that
can wrap below sizeof(struct scsi_lun), and the residual comparison
would then underflow.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Shao Mingyin <shao.mingyin@xxxxxxxxxx>
---
v2:
  - allocate the buffer with kzalloc() instead of kmalloc().
  - reword the commit message: scsi_execute_cmd() zeroes the residual
    tail, so the buffer is not simply left uninitialized.
  - keep the residual check, which is what catches a transfer that
    stopped inside the LUN list header.
  - also reject a length that cannot hold the LUN header, so the
    residual comparison cannot underflow when the device-reported
    length wraps on the retry path.
  - spell out that the check requires the full eight byte header.

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 52f3b525d2fd..e6aac7310537 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1475,10 +1475,11 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
 		.total_allowed = 3,
 		.failure_definitions = failure_defs,
 	};
+	int ret = 0, resid = 0;
 	const struct scsi_exec_args exec_args = {
+		.resid = &resid,
 		.failures = &failures,
 	};
-	int ret = 0;
 
 	/*
 	 * Only support SCSI-3 and up devices if BLIST_NOREPORTLUN is not set.
@@ -1516,7 +1517,7 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
 	 */
 	length = (511 + 1) * sizeof(struct scsi_lun);
 retry:
-	lun_data = kmalloc(length, GFP_KERNEL);
+	lun_data = kzalloc(length, GFP_KERNEL);
 	if (!lun_data) {
 		printk(ALLOC_FAILURE_MSG, __func__);
 		goto out;
@@ -1567,6 +1568,19 @@ static int scsi_report_lun_scan(struct Scsi_Host *shost,
 		goto out_err;
 	}
 
+	/*
+	 * Reject a transfer that did not fill in the whole LUN list header.
+	 * Also reject a length that cannot even hold the header: the retry
+	 * path computes it from the device-reported value with an addition
+	 * that can wrap below sizeof(struct scsi_lun), in which case the
+	 * residual comparison below would underflow.
+	 */
+	if (length < sizeof(struct scsi_lun) ||
+	    resid > length - sizeof(struct scsi_lun)) {
+		ret = 1;
+		goto out_err;
+	}
+
 	/*
 	 * Get the length from the first four bytes of lun_data.
 	 */