| Message ID | 20221213005243.2727877-2-shinichiro.kawasaki@wdc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: mpi3mr: fix issues found by KASAN | expand |
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 > 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). > rval = 0; > out: > kfree(alltgt_info); > -- > 2.37.1 >
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.
On Dec 13, 2022 / 07:17, Shinichiro Kawasaki wrote: > 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 [...] > > > 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. I've posted v3 using min_entrylen as you suggested. Also I added two more patches to the series, based on my understanding on the three points I had noted. Your review will be appreciated.
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; + } + 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); rval = 0; out: kfree(alltgt_info);
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> --- drivers/scsi/mpi3mr/mpi3mr_app.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)