Re: [PATCH 7/7] scsi: sd_zbc: define storage element management operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] scsi: sd_zbc: heap OOB read from buf via unvalidated nr_descs
- [High] scsi: sd_zbc: heap OOB leak caused by overwriting caller capacity with nr_descs
- [Medium] scsi: sd_zbc: hardcoded 4KB buffer permanently limits storage elements to 127
- [Medium] scsi: sd_zbc: undefined shift via ilog2(0) when zone_blocks is uninitialized
- [Low] build: invalid &NULL assignment for se_ops in non-zoned configs
--

commit dc87627d74d92a84dc8259e4c75a364fd39dae85
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: sd_zbc: define storage element management operations

This commit defines the storage element management operations using
struct blk_storage_elements_ops. These operations are intended for SMR disks
that support the storage element depopulation feature.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b0d2542fd3172..0a252739f5945 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3939,6 +3939,7 @@ static const struct block_device_operations sd_fops = {
>  	.get_unique_id		= sd_get_unique_id,
>  	.free_disk		= scsi_disk_free_disk,
>  	.pr_ops			= &sd_pr_ops,
> +	.se_ops			= &sd_zbc_se_ops,
>  };

[Severity: Low]
Does this cause a build failure when CONFIG_BLK_DEV_ZONED is disabled? Since
sd_zbc_se_ops is defined as NULL in sd.h for non-zoned configs, this expands
to &NULL which is syntactically invalid.

[ ... ]

> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 1bf5f7ea0d9c5..56e8456cd0eca 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -548,6 +548,216 @@ static void sd_zbc_print_zones(struct scsi_disk *sdkp)
[ ... ]
> +static void sd_zbc_parse_storage_element(struct scsi_disk *sdkp, u8 *desc,
> +					 struct blk_storage_element *element)
> +{
> +	struct scsi_device *sdp = sdkp->device;
> +	u64 capacity;
> +
> +	element->id = get_unaligned_be32(&desc[4]);
> +
> +	switch (desc[14]) {
> +	case SCSI_PHYS_ELEM_TYPE_ALL_ACCESS_STORAGE:
> +		element->paired_id = 0;
> +		element->type = BLK_SE_TYPE_RDWR;
> +		capacity = get_unaligned_be64(&desc[16]);
> +		element->nr_zones = logical_to_sectors(sdp, capacity) >>
> +			ilog2(sd_zbc_zone_sectors(sdkp));

[Severity: Medium]
Does this risk an undefined shift on architectures where ilog2(0) faults? If
this function is called on a device where modify_zones_supported is true but
zone_blocks is zero (such as following a zoned revalidation failure),
sd_zbc_zone_sectors(sdkp) would return 0.

[ ... ]

> +static int sd_zbc_report_storage_elements(struct gendisk *disk,
> +					  unsigned int *nr_elements,
> +					  struct blk_storage_element *elements)
> +{
[ ... ]
> +	memset(cmd, 0, 16);
> +	cmd[0] = SERVICE_ACTION_IN_16;
> +	cmd[1] = SAI_GET_PHYSICAL_ELEMENT_STATUS;
> +	put_unaligned_be32(SD_ZBC_STORAGE_ELEMENTS_BUF_SIZE, &cmd[10]);

[Severity: Medium]
Does this hardcoded 4KB allocation length permanently limit the number of
reported storage elements to 127? There appears to be no mechanism to query
elements iteratively, which limits functionality for devices with a larger
number of elements.

[ ... ]

> +	nr_descs = get_unaligned_be32(&buf[4]);
> +	if (!nr_descs) {
> +		sd_printk(KERN_ERR, sdkp,
> +			"Invalid number of reported phys element descriptors\n");
> +		ret = -EIO;
> +		goto free_buf;
> +	}
> +
> +	desc = &buf[32];
> +	for (i = 0; i < min(*nr_elements, nr_descs); i++, desc += 32)
> +		sd_zbc_parse_storage_element(sdkp, desc, &elements[i]);

[Severity: High]
Can this loop read out-of-bounds of the 4096-byte buf? The driver extracts
nr_descs directly from the device payload without validating it against the
buffer size limit. If a device returns a large nr_descs, this loop will
advance the desc pointer past the end of the allocated buf.

> +	*nr_elements = nr_descs;

[Severity: High]
Can this unconditionally overwrite the caller-provided array capacity
(*nr_elements) with the device-reported count? If the caller provided a
smaller array capacity than nr_descs, returning the larger nr_descs can
trick the caller into copying uninitialized out-of-bounds heap memory to
userspace.

> +
> +free_buf:
> +	kfree(buf);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005094632.580753-1-dlemoal@kernel.org?part=7