Re: [PATCH 02/37] scsi: mpt3sas: Validate RAID configuration page length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: mpt3sas: u16 truncation of config_page_sz in _config_request()
--

commit c2be3bad5af4de88c7b5227eeaab81b3e61723ef
Author: Shivasharan S <shivasharan.srikanteshwara@broadcom.com>

scsi: mpt3sas: Validate RAID configuration page length

This commit validates that the RAID configuration page length is large enough
to hold Mpi2RaidConfigurationPage0_t before allocating the buffer, and bounds
the element loop by the number of elements that fit in the allocated buffer.

> diff --git a/drivers/scsi/mpt3sas/mpt3sas_config.c b/drivers/scsi/mpt3sas/mpt3sas_config.c
> index b0d5ef8936008..75312d43e3862 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_config.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_config.c
> @@ -2655,6 +2655,7 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  	int config_num;
>  	u16 element_type;
>  	u16 phys_disk_dev_handle;
> +	int max_num_elements;
>  
>  	*volume_handle = 0;
>  	memset(&mpi_request, 0, sizeof(Mpi2ConfigRequest_t));
> @@ -2672,6 +2673,13 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  
>  	mpi_request.Action = MPI2_CONFIG_ACTION_PAGE_READ_CURRENT;
>  	config_page_sz = (le16_to_cpu(mpi_reply.ExtPageLength) * 4);
> +	if (config_page_sz < offsetof(Mpi2RaidConfigurationPage0_t,
> +				      ConfigElement)) {
> +		ioc_err(ioc, "%s: RAID configuration page length (%d) is too small\n",
> +			__func__, config_page_sz);
> +		r = -EIO;
> +		goto out;
> +	}
>  	config_page = kmalloc(config_page_sz, GFP_KERNEL);
>  	if (!config_page) {
>  		r = -1;
> @@ -2692,7 +2700,11 @@ mpt3sas_config_get_volume_handle(struct MPT3SAS_ADAPTER *ioc, u16 pd_handle,
>  		    MPI2_IOCSTATUS_MASK;
>  		if (ioc_status != MPI2_IOCSTATUS_SUCCESS)
>  			goto out;
> -		for (i = 0; i < config_page->NumElements; i++) {
> +		max_num_elements = (config_page_sz -
> +		    offsetof(Mpi2RaidConfigurationPage0_t, ConfigElement)) /
> +		    sizeof(config_page->ConfigElement[0]);
> +		for (i = 0; i < config_page->NumElements &&
> +		     i < max_num_elements; i++) {

[Severity: Medium]
Can this read uninitialized heap memory from config_page when ExtPageLength is
0x4000 or larger?

In mpt3sas_config_get_volume_handle(), config_page_sz is a 32-bit int computed
as le16_to_cpu(mpi_reply.ExtPageLength) * 4, which can reach up to 262140.
The new check only enforces a lower bound before calling kmalloc(), which
allocates uninitialized memory.

When mpt3sas_config_get_volume_handle() then calls _config_request(), the
size is truncated to u16 both in the parameter list and in mem.sz:

drivers/scsi/mpt3sas/mpt3sas_config.c:_config_request() {
	...
	static int
	_config_request(struct MPT3SAS_ADAPTER *ioc, Mpi2ConfigRequest_t
		*mpi_request, Mpi2ConfigReply_t *mpi_reply, int timeout,
		void *config_page, u16 config_page_sz)
	...
		if (mpi_request->Header.PageLength)
			mem.sz = mpi_request->Header.PageLength * 4;
		else
			mem.sz = le16_to_cpu(mpi_reply->ExtPageLength) * 4;
}

Inside _config_request(), only (u16)config_page_sz bytes of config_page are
zeroed with memset() and populated with memcpy().

If ExtPageLength is between 0x4000 and 0x400b, (u16)config_page_sz wraps to
between 0 and 44 bytes, so _config_request() never initializes
config_page->NumElements, config_page->ConfigElement[], or (when wrapped to
11 bytes or less) config_page->ConfigNum. Even for larger values where
ExtPageLength >= 0x400c, max_num_elements is computed from the untruncated
32-bit config_page_sz, allowing the loop to read ConfigElement[i] entries
beyond the (u16)config_page_sz bytes actually populated by _config_request().

Also, because _config_request() overwrites mpi_reply on each iteration of the
while (1) loop and recomputes mem.sz from the new mpi_reply.ExtPageLength on
the next pass, a smaller ExtPageLength on a subsequent iteration would copy
fewer bytes than the initial config_page_sz used for max_num_elements.

Should mpt3sas_config_get_volume_handle() also reject config_page_sz > U16_MAX
(or allocate with kzalloc()) so that config_page_sz cannot truncate when
passed to _config_request()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1793925772-1303-1-git-send-email-shivasharan.srikanteshwara@broadcom.com?part=2