[PATCH] scsi: target: bound VPD identifier formatting

[PATCH] scsi: target: bound VPD identifier formatting

[Mark Amirkan via B4 Relay]

From: Mark Amirkan <markdamirkan@gmail.com>

transport_set_vpd_ident() formats device-provided VPD page 0x83
identifiers into the 254-byte t10_vpd::device_identifier array without
checking whether the result fits.

A binary identifier emits one type character followed by two hexadecimal
characters per input byte.  A 148-byte identifier therefore emits 297
characters, eventually writing beyond the 296-byte t10_vpd allocation.

Reject binary identifiers longer than 126 bytes and apply the equivalent
destination bound to ASCII and UTF-8 identifiers.  Explicitly terminate
accepted identifiers.  Rejecting instead of truncating avoids creating a
false device identity.

The write was reproduced with generic KASAN on arm64 Linux 7.2-rc7 using
a complete 168-byte VPD response.  With the same input, the fixed kernel
retains the valid NAA descriptor, skips the 148-byte vendor-specific
descriptor, enables the pSCSI backstore, and produces no KASAN report.
Seven boundary tests pass.

The demonstrated path requires a device-provided response and privileged
pSCSI configuration.  No claim is made about exploitability or
unprivileged reachability.

The tested source reproducer is available privately on request.

Fixes: c66ac9db8d4a ("[SCSI] target: Add LIO target core v4.0.0-rc6")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.6
Signed-off-by: Mark Amirkan <markdamirkan@gmail.com>
---
 drivers/target/target_core_transport.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index dcfe9459..26a0bb66 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1335,6 +1335,14 @@ transport_set_vpd_ident(struct t10_vpd *vpd, unsigned char *page_83)
 	vpd->device_identifier_code_set = (page_83[0] & 0x0f);
 	switch (vpd->device_identifier_code_set) {
 	case 0x01: /* Binary */
+		/*
+		 * Reserve one character for the type and one for the NUL;
+		 * each binary byte expands to two hex characters.
+		 */
+		if (page_83[3] >
+		    (sizeof(vpd->device_identifier) - 2) / 2)
+			return -EINVAL;
+
 		vpd->device_identifier[j++] =
 				hex_str[vpd->device_identifier_type];
 		while (i < (4 + page_83[3])) {
@@ -1344,11 +1352,16 @@ transport_set_vpd_ident(struct t10_vpd *vpd, unsigned char *page_83)
 				hex_str[page_83[i] & 0x0f];
 			i++;
 		}
+		vpd->device_identifier[j] = '\0';
 		break;
 	case 0x02: /* ASCII */
 	case 0x03: /* UTF-8 */
+		if (page_83[3] >= sizeof(vpd->device_identifier))
+			return -EINVAL;
+
 		while (i < (4 + page_83[3]))
 			vpd->device_identifier[j++] = page_83[i++];
+		vpd->device_identifier[j] = '\0';
 		break;
 	default:
 		break;

---
base-commit: 3a0dd7ba4f44cdc116d83712f61e7c1a95be3588
change-id: 20260818-pscsi-vpd-ident-bounds-b5ffb129754d

Best regards,
--  
Mark Amirkan <markdamirkan@gmail.com>

Re: [PATCH] scsi: target: bound VPD identifier formatting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] pscsi_get_inquiry_vpd_device_ident() lacks bounds checking against the allocated buffer size when parsing VPD Identification descriptors.
- [High] Leaked bios in pscsi_execute_cmd() error path.
- [Medium] Missing NUL-termination enforcement in pscsi_get_inquiry_vpd_serial() leads to an out-of-bounds read via unbounded snprintf().
--

--- Patch [1]: [PATCH] scsi: target: bound VPD identifier formatting ---
commit 63526716e874620c72b9c2ea7622a0eea0ce5c5e
Author: Mark Amirkan <markdamirkan@gmail.com>

scsi: target: bound VPD identifier formatting

This patch adds bounds checking to transport_set_vpd_ident() when
formatting device-provided VPD page 0x83 identifiers into the fixed-size
t10_vpd::device_identifier array. It rejects binary, ASCII, and UTF-8
identifiers that exceed the allocation limits to avoid writing past the
296-byte boundary.

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index dcfe945949167..26a0bb66c2530 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1335,6 +1335,14 @@ transport_set_vpd_ident(struct t10_vpd *vpd, unsigned char *page_83)
>  	vpd->device_identifier_code_set = (page_83[0] & 0x0f);
>  	switch (vpd->device_identifier_code_set) {
>  	case 0x01: /* Binary */
> +		/*
> +		 * Reserve one character for the type and one for the NUL;
> +		 * each binary byte expands to two hex characters.
> +		 */
> +		if (page_83[3] >
> +		    (sizeof(vpd->device_identifier) - 2) / 2)
> +			return -EINVAL;
> +
>  		vpd->device_identifier[j++] =
>  				hex_str[vpd->device_identifier_type];
>  		while (i < (4 + page_83[3])) {
> @@ -1344,11 +1352,16 @@ transport_set_vpd_ident(struct t10_vpd *vpd, unsigned char *page_83)
>  				hex_str[page_83[i] & 0x0f];
>  			i++;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does the caller pass a properly
bounded page_83 pointer into this function?

Looking at pscsi_get_inquiry_vpd_device_ident() in target_core_pscsi.c, the loop
blindly advances the buffer offset using unvalidated lengths from the device:

drivers/target/target_core_pscsi.c:pscsi_get_inquiry_vpd_device_ident() {
    ...
    page_len = get_unaligned_be16(&buf[2]);
    while (page_len > 0) {
        page_83 = &buf[off];
        ident_len = page_83[3];
    ...
}

If page_len or ident_len exceed the 254-byte allocation limit for buf, couldn't
the while loop here in transport_set_vpd_ident() read out-of-bounds heap
memory when accessing page_83[i]?

[ ... ]

While reviewing the VPD handling, I also noticed a couple of other pre-existing
issues in the related target_core_pscsi.c code:

[Severity: Medium]
This is a pre-existing issue, but is there a risk of an out-of-bounds read
in pscsi_get_inquiry_vpd_serial() when parsing an untrusted buffer?

drivers/target/target_core_pscsi.c:pscsi_get_inquiry_vpd_serial() {
    ...
    snprintf(&wwn->unit_serial[0], INQUIRY_VPD_SERIAL_LEN, "%s", &buf[4]);
    ...
}

If a device returns 254 bytes of non-NUL characters for the INQUIRY response,
won't the %s format specifier cause snprintf() to read past the end of the
254-byte buf allocation up to the INQUIRY_VPD_SERIAL_LEN limit?

[Severity: High]
This is also a pre-existing issue, but does the error path in
pscsi_execute_cmd() leak bio structures?

drivers/target/target_core_pscsi.c:pscsi_execute_cmd() {
    ...
    if (scmd->cmd_len > sizeof(scmd->cmnd)) {
        ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
        goto fail_put_request;
    }
    ...
fail_put_request:
    blk_mq_free_request(req);
    return ret;
}

If pscsi_map_sg() successfully maps the scatterlist and allocates bios via
bio_kmalloc() attached to req->bio, and then the subsequent CDB length check
fails, doesn't blk_mq_free_request() free the request state without freeing the
attached bios? Could an unprivileged remote initiator repeatedly send
variable-length CDBs greater than 32 bytes to trigger this path and exhaust
memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-pscsi-vpd-ident-bounds-v1-1-ba28dcb0af08@gmail.com?part=1