| Message ID | 20210722033439.26550-2-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | UFS patches for kernel v5.15 | expand |
> > If param_offset > buff_len then the memcpy() statement in > ufshcd_read_desc_param() corrupts memory since it copies > 256 + buff_len - param_offset bytes into a buffer with size buff_len. > Since param_offset < 256 this results in writing past the bound of the output > buffer. > > Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during > memcpy") > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Avri Altman <avri.altman@wdc.com> Your fix is fine IMO. However, the root cause of this weird bug is that rpmb has its own unit descriptor, But ufshcd_map_desc_id_to_length doesn't accept index as argument, and returned 0x2d instead of 0x23, as it should. Thanks, Avri > --- > drivers/scsi/ufs/ufshcd.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index > 064a44e628d6..6c251afe65f9 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3418,9 +3418,11 @@ int ufshcd_read_desc_param(struct ufs_hba > *hba, > > if (is_kmalloc) { > /* Make sure we don't copy more data than available */ > - if (param_offset + param_size > buff_len) > - param_size = buff_len - param_offset; > - memcpy(param_read_buf, &desc_buf[param_offset], param_size); > + if (param_offset >= buff_len) > + ret = -EINVAL; > + else > + memcpy(param_read_buf, &desc_buf[param_offset], > + min_t(u32, param_size, buff_len - > + param_offset)); > } > out: > if (is_kmalloc)
Hi Bart, >If param_offset > buff_len then the memcpy() statement in >ufshcd_read_desc_param() corrupts memory since it copies >256 + buff_len - param_offset bytes into a buffer with size buff_len. >Since param_offset < 256 this results in writing past the bound of the >output buffer. Reviewed-by: Daejun Park <daejun7.park@samsung.com> Thanks, Daejun
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 064a44e628d6..6c251afe65f9 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3418,9 +3418,11 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, if (is_kmalloc) { /* Make sure we don't copy more data than available */ - if (param_offset + param_size > buff_len) - param_size = buff_len - param_offset; - memcpy(param_read_buf, &desc_buf[param_offset], param_size); + if (param_offset >= buff_len) + ret = -EINVAL; + else + memcpy(param_read_buf, &desc_buf[param_offset], + min_t(u32, param_size, buff_len - param_offset)); } out: if (is_kmalloc)
If param_offset > buff_len then the memcpy() statement in ufshcd_read_desc_param() corrupts memory since it copies 256 + buff_len - param_offset bytes into a buffer with size buff_len. Since param_offset < 256 this results in writing past the bound of the output buffer. Fixes: cbe193f6f093 ("scsi: ufs: Fix potential NULL pointer access during memcpy") Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/ufs/ufshcd.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)