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

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 <[email protected]>

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/[email protected]?part=1