| Message ID | 20230214005019.1897251-2-shinichiro.kawasaki@wdc.com (mailing list archive) |
|---|---|
| State | Deferred |
| Headers | show |
| Series | scsi: mpi3mr: fix issues found by KASAN | expand |
On Mon, Feb 13, 2023 at 5:50 PM Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> wrote: > > The function mpi3mr_get_all_tgt_info has four issues as follow. > > 1) It calculates valid entry length in alltgt_info assuming the header > part of the struct mpi3mr_device_map_info would equal to sizeof(u32). > The correct size is sizeof(u64). > 2) When it calculates the valid entry length kern_entrylen, it excludes > one entry by subtracting 1 from num_devices. > 3) It copies num_device by calling memcpy. Substitution is enough. > 4) It does not specify the calculated length to sg_copy_from_buffer(). > Instead, it specifies the payload length which is larger than the > alltgt_info size. It causes "BUG: KASAN: slab-out-of-bounds". > > Fix the issues by using the correct header size, removing the subtract > from num_devices, replacing the memcpy with substitution and specifying > the correct length to sg_copy_from_buffer. > > Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands") > Cc: stable@vger.kernel.org > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> Acked-by: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com> > > --- > drivers/scsi/mpi3mr/mpi3mr_app.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c > index 9baac224b213..72054e3a26cb 100644 > --- a/drivers/scsi/mpi3mr/mpi3mr_app.c > +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c > @@ -312,7 +312,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > num_devices++; > spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); > > - if ((job->request_payload.payload_len == sizeof(u32)) || > + 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, > @@ -320,14 +320,14 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > return 0; > } > > - kern_entrylen = (num_devices - 1) * sizeof(*devmap_info); > - size = sizeof(*alltgt_info) + kern_entrylen; > + kern_entrylen = num_devices * sizeof(*devmap_info); > + size = sizeof(u64) + kern_entrylen; > alltgt_info = kzalloc(size, GFP_KERNEL); > if (!alltgt_info) > return -ENOMEM; > > devmap_info = alltgt_info->dmi; > - memset((u8 *)devmap_info, 0xFF, (kern_entrylen + sizeof(*devmap_info))); > + memset((u8 *)devmap_info, 0xFF, kern_entrylen); > spin_lock_irqsave(&mrioc->tgtdev_lock, flags); > list_for_each_entry(tgtdev, &mrioc->tgtdev_list, list) { > if (i < num_devices) { > @@ -344,9 +344,10 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, > num_devices = i; > spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); > > - memcpy(&alltgt_info->num_devices, &num_devices, sizeof(num_devices)); > + alltgt_info->num_devices = num_devices; > > - usr_entrylen = (job->request_payload.payload_len - sizeof(u32)) / sizeof(*devmap_info); > + usr_entrylen = (job->request_payload.payload_len - sizeof(u64)) / > + sizeof(*devmap_info); > usr_entrylen *= sizeof(*devmap_info); > min_entrylen = min(usr_entrylen, kern_entrylen); > if (min_entrylen && (!memcpy(&alltgt_info->dmi, devmap_info, min_entrylen))) { > @@ -358,7 +359,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, (min_entrylen + sizeof(u64))); > rval = 0; > out: > kfree(alltgt_info); > -- > 2.38.1 >
diff --git a/drivers/scsi/mpi3mr/mpi3mr_app.c b/drivers/scsi/mpi3mr/mpi3mr_app.c index 9baac224b213..72054e3a26cb 100644 --- a/drivers/scsi/mpi3mr/mpi3mr_app.c +++ b/drivers/scsi/mpi3mr/mpi3mr_app.c @@ -312,7 +312,7 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, num_devices++; spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); - if ((job->request_payload.payload_len == sizeof(u32)) || + 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, @@ -320,14 +320,14 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, return 0; } - kern_entrylen = (num_devices - 1) * sizeof(*devmap_info); - size = sizeof(*alltgt_info) + kern_entrylen; + kern_entrylen = num_devices * sizeof(*devmap_info); + size = sizeof(u64) + kern_entrylen; alltgt_info = kzalloc(size, GFP_KERNEL); if (!alltgt_info) return -ENOMEM; devmap_info = alltgt_info->dmi; - memset((u8 *)devmap_info, 0xFF, (kern_entrylen + sizeof(*devmap_info))); + memset((u8 *)devmap_info, 0xFF, kern_entrylen); spin_lock_irqsave(&mrioc->tgtdev_lock, flags); list_for_each_entry(tgtdev, &mrioc->tgtdev_list, list) { if (i < num_devices) { @@ -344,9 +344,10 @@ static long mpi3mr_get_all_tgt_info(struct mpi3mr_ioc *mrioc, num_devices = i; spin_unlock_irqrestore(&mrioc->tgtdev_lock, flags); - memcpy(&alltgt_info->num_devices, &num_devices, sizeof(num_devices)); + alltgt_info->num_devices = num_devices; - usr_entrylen = (job->request_payload.payload_len - sizeof(u32)) / sizeof(*devmap_info); + usr_entrylen = (job->request_payload.payload_len - sizeof(u64)) / + sizeof(*devmap_info); usr_entrylen *= sizeof(*devmap_info); min_entrylen = min(usr_entrylen, kern_entrylen); if (min_entrylen && (!memcpy(&alltgt_info->dmi, devmap_info, min_entrylen))) { @@ -358,7 +359,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, (min_entrylen + sizeof(u64))); rval = 0; out: kfree(alltgt_info);
The function mpi3mr_get_all_tgt_info has four issues as follow. 1) It calculates valid entry length in alltgt_info assuming the header part of the struct mpi3mr_device_map_info would equal to sizeof(u32). The correct size is sizeof(u64). 2) When it calculates the valid entry length kern_entrylen, it excludes one entry by subtracting 1 from num_devices. 3) It copies num_device by calling memcpy. Substitution is enough. 4) It does not specify the calculated length to sg_copy_from_buffer(). Instead, it specifies the payload length which is larger than the alltgt_info size. It causes "BUG: KASAN: slab-out-of-bounds". Fix the issues by using the correct header size, removing the subtract from num_devices, replacing the memcpy with substitution and specifying the correct length to sg_copy_from_buffer. Fixes: f5e6d5a34376 ("scsi: mpi3mr: Add support for driver commands") Cc: stable@vger.kernel.org Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com> --- drivers/scsi/mpi3mr/mpi3mr_app.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)