* Re: [PATCH] scsi: hpsa: fix divide-by-zero in hpsa_scsi_ioaccel_raid_map
2026-08-14 9:00 [PATCH] scsi: hpsa: fix divide-by-zero in hpsa_scsi_ioaccel_raid_map Haotian Zhang
@ 2026-08-14 9:42 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-08-14 9:42 UTC (permalink / raw)
To: Haotian Zhang; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 2+ messages in thread