Re: [PATCH 15/17] mpi3mr: zero out diagnostic buffer status memory

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

Pre-existing issues:
- [High] An integer overflow in the bounds check of `mpi3mr_bsg_upload_hdb` allows a malicious user to bypass the check and read arbitrary kernel memory.
--

--- Patch [15]: [PATCH 15/17] mpi3mr: zero out diagnostic buffer status memory ---
commit 76872b40f16cd1ab1c93f0a03cc6bd1fb6b3db94
Author: Chandrakanth Patil <[email protected]>

mpi3mr: zero out diagnostic buffer status memory
    
Memory allocated for the BSG diagnostic buffer status is not zero'd
before it is copied back to user space. Several fields in this
structure are reserved and never written by the driver. Use kzalloc
instead of kmalloc to zero out the allocated memory.
    
Fixes: 78b506984ebe ("scsi: mpi3mr: Add ioctl support for HDB")
Signed-off-by: Chandrakanth Patil <[email protected]>

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> index f5b48c95cf055..0cdcb8d236d37 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> @@ -1244,7 +1244,7 @@ static long mpi3mr_bsg_query_hdb(struct mpi3mr_ioc *mrioc,
>  
>  	length = (sizeof(*hbd_status) + ((MPI3MR_MAX_NUM_HDB - 1) *
>  		    sizeof(*hbd_status_entry)));
> -	hbd_status = kmalloc(length, GFP_KERNEL);
> +	hbd_status = kzalloc(length, GFP_KERNEL);
>  	if (!hbd_status)
>  		return -ENOMEM;
>  	hbd_status_entry = &hbd_status->entry[0];

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential integer overflow in mpi3mr_bsg_upload_hdb():

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_bsg_upload_hdb() {
    ...
	if ((upload_hdb.start_offset + upload_hdb.length) > diag_buffer->size) {
		dprint_bsg_err(mrioc,
		    "%s: invalid start offset %d, length %d for type %d\n",
		    __func__, upload_hdb.start_offset, upload_hdb.length,
		    upload_hdb.buf_type);
		return -EINVAL;
	}
    ...
}

Can upload_hdb.start_offset and upload_hdb.length wrap around during addition?

Since they are 32-bit unsigned integers controlled by user space via the BSG
interface, a large start_offset (e.g. 0xFFFFFFF0) and a small length
(e.g. 0x20) could wrap to a sum that passes the bounds check against
diag_buffer->size.

Would this allow a malicious user to bypass the check and read arbitrary kernel
memory when the subsequent copy occurs?

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