| Message ID | 20210111044443.1405049-1-jaegeuk@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: should not override buffer lengh | expand |
Hi Jaegeuk, I think the problem is that func ufshcd_read_desc_param() is not expecting one access unsupported descriptors on all W-LUs, not just RPMB LU. If we can get the right buf_len from func ufshcd_map_desc_id_to_length(), the issue won't happen. - https://lore.kernel.org/patchwork/patch/1323421/. What do you think if we update ufshcd_map_desc_id_to_length(add one param - desc_index) so that it can tell the correct buf_len in case of W-LUs? Thanks, Can Guo. On 2021-01-11 12:44, Jaegeuk Kim wrote: > From: Jaegeuk Kim <jaegeuk@google.com> > > Kernel stack violation when getting unit_descriptor/wb_buf_alloc_units > from > rpmb lun. The reason is the unit descriptor length is different per LU. > > The lengh of Normal LU is 45, while the one of rpmb LU is 35. > > int ufshcd_read_desc_param(struct ufs_hba *hba, ...) > { > param_offset=41; > param_size=4; > buff_len=45; > ... > buff_len=35 by rpmb LU; > > 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; > --> param_size = 250; > memcpy(param_read_buf, &desc_buf[param_offset], param_size); > --> memcpy(param_read_buf, desc_buf+41, 250); > > [ 141.868974][ T9174] Kernel panic - not syncing: stack-protector: > Kernel stack is corrupted in: wb_buf_alloc_units_show+0x11c/0x11c > } > } > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > drivers/scsi/ufs/ufshcd.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 2a715f13fe1d..722697b57777 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3293,8 +3293,12 @@ 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; > + if (param_offset + param_size > buff_len) { > + if (buff_len > param_offset) > + param_size = buff_len - param_offset; > + else > + param_size = 0; > + } > memcpy(param_read_buf, &desc_buf[param_offset], param_size); > } > out:
Sorry, typo corrected. Hi Jaegeuk, I think the problem is that func ufshcd_read_desc_param() is not expecting one access unsupported descriptors on RPMB LU. If we can get the right buf_len from func ufshcd_map_desc_id_to_length(), the issue won't happen. - https://lore.kernel.org/patchwork/patch/1323421/. What do you think if we update ufshcd_map_desc_id_to_length(add one param - desc_index) so that it can tell the correct buf_len in case of RPMB LU? Thanks, Can Guo. > On 2021-01-11 12:44, Jaegeuk Kim wrote: >> From: Jaegeuk Kim <jaegeuk@google.com> >> >> Kernel stack violation when getting unit_descriptor/wb_buf_alloc_units >> from >> rpmb lun. The reason is the unit descriptor length is different per >> LU. >> >> The lengh of Normal LU is 45, while the one of rpmb LU is 35. >> >> int ufshcd_read_desc_param(struct ufs_hba *hba, ...) >> { >> param_offset=41; >> param_size=4; >> buff_len=45; >> ... >> buff_len=35 by rpmb LU; >> >> 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; >> --> param_size = 250; >> memcpy(param_read_buf, &desc_buf[param_offset], param_size); >> --> memcpy(param_read_buf, desc_buf+41, 250); >> >> [ 141.868974][ T9174] Kernel panic - not syncing: stack-protector: >> Kernel stack is corrupted in: wb_buf_alloc_units_show+0x11c/0x11c >> } >> } >> >> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index 2a715f13fe1d..722697b57777 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -3293,8 +3293,12 @@ 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; >> + if (param_offset + param_size > buff_len) { >> + if (buff_len > param_offset) >> + param_size = buff_len - param_offset; >> + else >> + param_size = 0; >> + } >> memcpy(param_read_buf, &desc_buf[param_offset], param_size); >> } >> out:
> > Sorry, typo corrected. > > Hi Jaegeuk, > > I think the problem is that func ufshcd_read_desc_param() is not > expecting > one access unsupported descriptors on RPMB LU. Correct. This is about wb introducing a new constraint: wb buffer is only allowed in lu 0..7. And this is why, IMHO, the fix should be in ufs_is_valid_unit_desc_lun, To include param offset, as it is only called in contingency of ufshcd_read_desc_param. Thanks, Avri
On 2021-01-11 16:15, Avri Altman wrote: >> >> Sorry, typo corrected. >> >> Hi Jaegeuk, >> >> I think the problem is that func ufshcd_read_desc_param() is not >> expecting >> one access unsupported descriptors on RPMB LU. > Correct. > This is about wb introducing a new constraint: wb buffer is only > allowed in lu 0..7. > And this is why, IMHO, the fix should be in ufs_is_valid_unit_desc_lun, > To include param offset, as it is only called in contingency of > ufshcd_read_desc_param. > > Thanks, > Avri Yeah... Fixing it in ufs-sysfs.c also works. Anyways, the math in ufshcd_read_desc_param is already complex. Let's fix it somewhere close to the source/initiator. Thanks, Can Guo.
On 01/11, Can Guo wrote: > On 2021-01-11 16:15, Avri Altman wrote: > > > > > > Sorry, typo corrected. > > > > > > Hi Jaegeuk, > > > > > > I think the problem is that func ufshcd_read_desc_param() is not > > > expecting > > > one access unsupported descriptors on RPMB LU. > > Correct. > > This is about wb introducing a new constraint: wb buffer is only > > allowed in lu 0..7. > > And this is why, IMHO, the fix should be in ufs_is_valid_unit_desc_lun, > > To include param offset, as it is only called in contingency of > > ufshcd_read_desc_param. > > > > Thanks, > > Avri > > Yeah... Fixing it in ufs-sysfs.c also works. Anyways, the math in > ufshcd_read_desc_param is already complex. Let's fix it somewhere > close to the source/initiator. Thank you, Can and Avri. I think fixing the lun check makese sense. Let me post v2. :) > > Thanks, > Can Guo.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 2a715f13fe1d..722697b57777 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3293,8 +3293,12 @@ 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; + if (param_offset + param_size > buff_len) { + if (buff_len > param_offset) + param_size = buff_len - param_offset; + else + param_size = 0; + } memcpy(param_read_buf, &desc_buf[param_offset], param_size); } out: