Re: [PATCH v2] scsi: alua: Use access state macros

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

Pre-existing issues:
- [High] Out-of-bounds heap read when parsing Target Port Group descriptors from a malformed RTPG response.
--

--- Patch [1]: [PATCH v2] scsi: alua: Use access state macros ---
commit 6e972b4dac6a9789e1cd5b0e5ccd315a61ef1227
Author: John Garry <[email protected]>

scsi: alua: Use access state macros

This commit replaces hardcoded bitwise operations with SCSI access state
macros when parsing Target Port Group descriptors in alua_rtpg().

> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 80ab0ff921d43..d81ec47a71402 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c

[ ... ]

> @@ -693,8 +694,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
>  				    !(tmp_pg->flags & ALUA_PG_RUNNING)) {
>  					struct alua_dh_data *h;
>  
> -					tmp_pg->state = desc[0] & 0x0f;
> -					tmp_pg->pref = desc[0] >> 7;
> +					tmp_pg->state = FIELD_GET(SCSI_ACCESS_STATE_MASK, desc[0]);
> +					tmp_pg->pref = FIELD_GET(SCSI_ACCESS_STATE_PREFERRED, desc[0]);

[Severity: High]
This is a pre-existing issue, but is there a risk of an out-of-bounds heap
read on the desc pointer here?

Looking at alua_rtpg() in drivers/scsi/device_handler/scsi_dh_alua.c, the
loop iterating over the descriptors evaluates the length returned by the
target:

for (k = tpg_desc_tbl_off, desc = buff + tpg_desc_tbl_off;
     k < len;
     k += off, desc += off) {
        u16 group_id = get_unaligned_be16(&desc[2]);

If the target returns a malformed REPORT TARGET PORT GROUPS response where
len indicates a truncated final descriptor (for example, fewer than 8 bytes
remain), the loop condition k < len still evaluates to true.

Could this cause the reads of desc[0], desc[2], and desc[7] to access
memory past the end of the kmalloc buffer if a target provides a crafted
response length?

>  					rcu_read_lock();
>  					list_for_each_entry_rcu(h,
>  						&tmp_pg->dh_list, node) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1