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

On Tue, Aug 18, 2026 at 11:51 PM <[email protected]> wrote:
>
> 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

Thanks for the review.

New issues:
- None. This patch specifically ensures diagnostic buffer status
memory is zeroed out using kzalloc() before copying to user space.

Pre-existing issues:
- The potential integer overflow in mpi3mr_bsg_upload_hdb() is a
pre-existing issue in the driver and will be addressed in a separate
follow-up patch series.