Re: [PATCH v2 13/17] mpi3mr: Fix buffer overflow in the BSG target device map

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

Pre-existing issues:
- [Medium] Pre-existing integer overflow in `mpi3mr_get_logdata` when calculating total buffer size
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition in `mpi3mr_get_all_tgt_info`
--

commit db64ca74e30e89ca553a673a670db46d0985b8fa
Author: Chandrakanth Patil <[email protected]>

    mpi3mr: Fix buffer overflow in the BSG target device map

This commit changes the size variable to size_t in mpi3mr_get_all_tgt_info
to fix a buffer overflow when calculating the target device map buffer size.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index cd772b2cb98ae..f5b48c95cf055 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -1466,7 +1466,8 @@ static long mpi3mr_bsg_pel_enable(struct mpi3mr_ioc *mrioc,
>  static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  	struct bsg_job *job)
>  {
> -	u16 num_devices = 0, i = 0, size;
> +	u16 num_devices = 0, i = 0;
> +	size_t size;
>  	unsigned long flags;
>  	struct mpi3mr_tgt_dev *tgtdev;
>  	struct mpi3mr_device_map_info *devmap_info = NULL;

[ ... ]

> @@ -1492,8 +1493,8 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
>  		return 0;
>  	}

[Severity: Medium]
This is a pre-existing issue, but is there a Time-of-Check to Time-of-Use
(TOCTOU) race condition here?

Right above this block, the code evaluates list_empty(&mrioc->tgtdev_list)
without holding a lock:

	if ((job->request_payload.payload_len <= sizeof(u64)) ||
		list_empty(&mrioc->tgtdev_list)) {
		sg_copy_from_buffer(job->request_payload.sg_list,
				    job->request_payload.sg_cnt,
				    &num_devices, sizeof(num_devices));
		return 0;
	}

If a target device is removed (e.g., hot unplug) right after the tgtdev_lock
is released (where num_devices was counted) but before this list_empty()
check, could we return a non-zero device count to user space without actually
populating the device mapping data array?

This might lead the application to read uninitialized memory from its own
buffer as valid device information.

> -	kern_entrylen = num_devices * sizeof(*devmap_info);
> -	size = sizeof(u64) + kern_entrylen;
> +	kern_entrylen = (uint32_t)num_devices * sizeof(*devmap_info);
> +	size = sizeof(u64) + (size_t)kern_entrylen;
>  	alltgt_info = kzalloc(size, GFP_KERNEL);
>  	if (!alltgt_info)
>  		return -ENOMEM;

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing the integer
overflow fix here, I noticed a similar pre-existing issue in
mpi3mr_get_logdata().

In mpi3mr_get_logdata(), the sz variable is declared as u16 and can
overflow when multiplied:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_get_logdata() {
    ...
	u16 sz, num_entries, entry_sz;
    ...
	sz = num_entries * entry_sz;
    ...
}

If the firmware advertises a reply_frame_size that makes entry_sz > 163 bytes
(e.g., a standard 256-byte frame), and we request MPI3MR_BSG_LOGDATA_MAX_ENTRIES
(400), could num_entries * entry_sz overflow the 16-bit sz variable?

This could silently truncate the amount of log data copied to user space,
causing the management tool to read uninitialized memory for the remaining
entries.

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