Re: [PATCH] scsi: hpsa: fix divide-by-zero in hpsa_scsi_ioaccel_raid_map

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability due to a double fetch of `map->strip_size`, which defeats the newly added divide-by-zero check.
- [Low] The patch calls an undeclared and undefined function `hpsa_turn_off_ioaccel_for_device()`, which will cause a build and linker failure.

Pre-existing issues:
- [Critical] The block address parsing for SCSI `READ_6` and `WRITE_6` commands ignores the top 5 bits of the 21-bit LBA, leading to silent data corruption.
- [High] The controller-provided `map->row_cnt` field is used as a modulo divisor without being validated against zero, leading to a divide-by-zero panic.
- [High] A 32-bit integer overflow when calculating `stripesize` can result in a divisor of 0, bypassing safety checks and leading to a divide-by-zero crash.
- [High] The driver blindly trusts controller-supplied values to enforce safety using multiple `BUG_ON()` assertions, creating trivial vectors where malformed firmware data can panic the host.
- [High] In `hpsa_figure_phys_disk_ptrs`, a signed integer overflow when computing `nraid_map_entries` can bypass the upper-bound array check, leading to a NULL pointer dereference on subsequent I/O.
--

--- Patch [1]: [PATCH] scsi: hpsa: fix divide-by-zero in hpsa_scsi_ioaccel_raid_map ---
commit 3421092ab4fbde9690cdb0b8f3136415015aac53
Author: Haotian Zhang <vulab@iscas.ac.cn>

scsi: hpsa: fix divide-by-zero in hpsa_scsi_ioaccel_raid_map

hpsa_scsi_ioaccel_raid_map() divides by blocks_per_row, the product of
data_disks_per_row and strip_size, two controller-supplied RAID map
fields that are never validated.  If either field is zero, the
unguarded division raises a divide-by-zero exception on every I/O to an
offload-enabled logical volume.

Check blocks_per_row for zero as soon as it is computed.  If it is
zero, turn off ioaccel for the device and return IO_ACCEL_INELIGIBLE.

> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -4444,6 +4444,10 @@ static int hpsa_scsi_ioaccel_raid_map(struct ctlr_info *h,
>  	/* calculate stripe information for the request */
>  	blocks_per_row = le16_to_cpu(map->data_disks_per_row) *
>  				le16_to_cpu(map->strip_size);
> +	if (blocks_per_row == 0) {
> +		hpsa_turn_off_ioaccel_for_device(dev);

[Severity: Low]
Does this break the build? The function hpsa_turn_off_ioaccel_for_device does
not appear to be declared or defined anywhere, which could lead to an
implicit function declaration and undefined reference at link time.

> +		return IO_ACCEL_INELIGIBLE;
> +	}
>  	strip_size = le16_to_cpu(map->strip_size);

[Severity: High]
Could this second fetch of map->strip_size reintroduce the divide-by-zero
issue? Since the I/O path executes locklessly with respect to device
configuration updates, a concurrent update could change map->strip_size to
zero right after the blocks_per_row check, causing the subsequent operations
using strip_size to fault.

[ ... ]

While reviewing this area, I noticed a few pre-existing issues in the same
driver that might be worth addressing:

drivers/scsi/hpsa.c:hpsa_scsi_ioaccel_raid_map() {
    ...
    map_row = ((u32)(first_row >> map->parity_rotation_shift)) %
              le16_to_cpu(map->row_cnt);

[Severity: High]
This isn't a bug introduced by this patch, but does this modulo operation
risk a divide-by-zero exception? The controller-provided map->row_cnt field
appears to be used here without being validated against zero.

[ ... ]

drivers/scsi/hpsa.c:hpsa_scsi_ioaccel_raid_map() {
    ...
    r5or6_blocks_per_row =
        le16_to_cpu(map->strip_size) *
        le16_to_cpu(map->data_disks_per_row);
    BUG_ON(r5or6_blocks_per_row == 0);
    stripesize = r5or6_blocks_per_row *
        le16_to_cpu(map->layout_map_count);

[Severity: High]
This is a pre-existing issue, but can this stripesize calculation silently
overflow to zero? Both r5or6_blocks_per_row and layout_map_count are
evaluated as signed 32-bit integers. If the controller provides specific
values (such as strip_size = 16384, data_disks_per_row = 8, and
layout_map_count = 32768), the product evaluates to exactly 2^32. This wraps
to 0 when stored in the u32 stripesize variable, causing a divide-by-zero
crash later.

[ ... ]

drivers/scsi/hpsa.c:hpsa_scsi_ioaccel_raid_map() {
    ...
    case HPSA_RAID_1:
        /* Handles load balance across RAID 1 members.
         * (2-drive R1 and R10 with even # of drives.)
         * Appropriate for SSDs, not optimal for HDDs
         */
        BUG_ON(le16_to_cpu(map->layout_map_count) != 2);

[Severity: High]
This is a pre-existing issue, but is it safe to use BUG_ON() to assert
structural properties of the raid_map? A buggy or compromised controller
could provide a mismatched layout map count, which would trigger this
assertion and panic the host immediately upon processing an I/O instead of
safely falling back to standard non-accelerated I/O.

[ ... ]

drivers/scsi/hpsa.c:hpsa_scsi_ioaccel_raid_map() {
    ...
    switch (cmd->cmnd[0]) {
    case WRITE_6:
        is_write = 1;
    case READ_6:
        first_block =
            (((u64) cmd->cmnd[2]) << 8) |
            cmd->cmnd[3];

[Severity: Critical]
This is a pre-existing issue, but does this LBA reconstruction silently
truncate the address? The standard 6-byte SCSI command format places the
upper 5 bits of the 21-bit LBA into byte 1 (cmd->cmnd[1]). By ignoring
byte 1 entirely, any application issuing a READ_6 or WRITE_6 command to an
offload-enabled device targeting an LBA greater than or equal to 65536 will
have the address truncated, resulting in operations against the wrong disk
region.

[ ... ]

drivers/scsi/hpsa.c:hpsa_figure_phys_disk_ptrs() {
    ...
    int nraid_map_entries = le16_to_cpu(map->row_cnt) *
                            le16_to_cpu(map->layout_map_count) *
                            total_disks_per_row;
    if (nraid_map_entries > RAID_MAP_MAX_ENTRIES) {

[Severity: High]
This isn't a bug introduced by this patch, but can this multiplication
overflow? Evaluated as a signed 32-bit integer, a maliciously crafted RAID
map from the controller could cause this product to exceed INT_MAX and wrap
to a negative value.

This would bypass the upper-bound check against RAID_MAP_MAX_ENTRIES,
leaving elements in the phys_disk array initialized as NULL. During I/O
submission, fetching and dereferencing this NULL pointer to access
ioaccel_handle will result in a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814090015.10207-1-vulab@iscas.ac.cn?part=1