Re: [PATCH v2 1/3] scsi: mpi3mr: fix alltgt_info copy size in mpi3mr_get_all_tgt_info

[Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>]

Hello Sathya, thanks for the comment.

On Dec 12, 2022 / 22:38, Sathya Prakash Veerichetty wrote:
> On Mon, Dec 12, 2022 at 5:52 PM Shin'ichiro Kawasaki
> <shinichiro.kawasaki@wdc.com> wrote:
> >
> > The function mpi3mr_get_all_tgt_info calculates size of alltgt_info and
> > allocate memory for it. After preparing valid data in alltgt_info, it
> > calls sg_copy_from_buffer to copy alltgt_info to job->request_payload,
> > specifying length of the payload as copy length. This length is larger
> > than the calculated alltgt_info size. It causes memory access to invalid
> > address and results in "BUG: KASAN: slab-out-of-bounds". The BUG was
> > observed during boot using systems with eHBA-9600. By updating the HBA
> > firmware to latest version 8.3.1.0 the BUG was not observed during boot,
> > but still observed when command "storcli2 /c0 show" is executed.
> >
> > Fix the BUG by specifying the calculated alltgt_info size as copy
> > length. Also check that the copy destination payload length is larger
> > than the copy length.
> >
> > Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> > Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> 
> Thanks for the patch, though this code needs a fix, the changes are
> not correct and needs modification.
> > ---
> >  drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > index 9baac224b213..2e35b0fece9c 100644
> > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c
> > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c
> > @@ -322,6 +322,13 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
> >
> >         kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
> >         size = sizeof(*alltgt_info) + kern_entrylen;
> > +
> > +       if (size > job->request_payload.payload_len) {
> > +               dprint_bsg_err(mrioc, "%s: payload length is too small\n",
> > +                              __func__);
> > +               return rval;
> > +       }
> > +
> This check is not needed, this is already handled by reducing the size
> to be copied to the given payload size

Ah, I see that min_entrylen is prepared to copy bytes smaller than the payload
size.

> >         alltgt_info = kzalloc(size, GFP_KERNEL);
> >         if (!alltgt_info)
> >                 return -ENOMEM;
> > @@ -358,7 +365,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc,
> >
> >         sg_copy_from_buffer(job->request_payload.sg_list,
> >                             job->request_payload.sg_cnt,
> > -                           alltgt_info, job->request_payload.payload_len);
> > +                           alltgt_info, size);
> instead of size, this should be min_entry_len+sizeof(u32).

Thanks for the comment. I read through mpi3mr_get_all_tgt_info() again. I still
have three unclear points. Your comments on them will be appreciated.

1) copy length

The pointer alltgt_info points to the struct below, which is defined in
include/uapi/scsi/scsi_bsg_mpi3mr.h (I refer kernel code at v6.1):

struct mpi3mr_all_tgt_info {
	__u16	num_devices;
	__u16	rsvd1;
	__u32	rsvd2;
	struct mpi3mr_device_map_info dmi[1];
};

When we copy "min_entrylen+sizeof(u32)", it looks for me that the struct is
copied partially. The expected length is as follows, isn't it?

  "min_entrylen + sizeof(u16) + sizeof(u16) + sizeof(u32)"

Regarding the min_entrylen, I find code in mpi3mr_get_all_tgt_info:

	usr_entrylen = (job->request_payload.payload_len - sizeof(u32)) / sizeof(*devmap_info);
	usr_entrylen *= sizeof(*devmap_info);
	min_entrylen = min(usr_entrylen, kern_entrylen);

The usr_entrylen calculation subtracts sizeof(u32). I guess the line also
needs change to subtract sizeof(u16) + sizeof(u16) + sizeof(u32).


2) kern_entrylen

usr_entrylen is compared with kern_entrylen to get min_etnrylen. And
kern_entrylen covers (num_devices - 1) entries:

	kern_entrylen = (num_devices - 1) * sizeof(*devmap_info);
	size = sizeof(*alltgt_info) + kern_entrylen;

Is it ok to cover only (num_devices - 1) for comparison with usr_entrylen?
Don't we need to cover all num_devices?


3) memcpy from devmap_info to alltgt_info->dmi

Also regarding the min_entrylen, I find a line below:

	if (min_entrylen && (!memcpy(&alltgt_info->dmi, devmap_info, min_entrylen))) {

The memcpy copies data from devmap_info to alltgt_inf->dmi, but it looks for me
that these two points to same address. Do we really need this memcpy?


I'm new to the mpi3mr driver. If I overlook or misunderstand anything, please
let me know.

-- 
Shin'ichiro Kawasaki