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

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

Pre-existing issues:
- [High] Uninitialized kernel heap memory parsing due to missing zero-initialization on reallocation.
- [High] Heap out-of-bounds read in the ALUA RTPG descriptor parsing loop due to missing bounds check.
--

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

scsi: alua: Use access state macros

Use the SCSI access state macros in alua_rtpg(), which is better than
hardcoding.

> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
> index 80ab0ff921d43..4258c48d93339 100644
> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
> @@ -693,8 +693,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing alua_rtpg(),
I noticed a potential issue with the buffer reallocation earlier in the
function:

    buff = kmalloc(bufflen, GFP_KERNEL);

If the subsequent submit_rtpg() succeeds but transfers fewer bytes than
requested (a short read), wouldn't the trailing portion of the buffer remain
uninitialized? Since the function parses up to the reported len, could this
cause it to parse uninitialized kernel heap memory if a malicious device
advertises a large length but returns a short response?

[Severity: High]
This is also a pre-existing issue, but looking at the descriptor parsing loop
just before the modified lines:

    for (k = tpg_desc_tbl_off, desc = buff + tpg_desc_tbl_off;
         k < len;
         k += off, desc += off) {
        ...
        off = 8 + (desc[7] * 4);
    }

Does the loop condition k < len ensure that a full 8-byte descriptor header
is available? If a device returns a truncated response where len - k < 8,
wouldn't the accesses to desc[0] through desc[7] read past the end of the heap
allocation?

>  				    !(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 = desc[0] & SCSI_ACCESS_STATE_MASK;
> +					tmp_pg->pref = desc[0] >> __fls(SCSI_ACCESS_STATE_PREFERRED);
>  					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