| Message ID | 1603346348-14149-1-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/1] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param() | expand |
> Since WB feature has been added, WB related sysfs entries can be accessed > even when an UFS device does not support WB feature. In that case, the > descriptors which are not supported by the UFS device may be wrongly > reported when they are accessed from their corrsponding sysfs entries. > Fix it by adding a sanity check of parameter offset against the actual > decriptor length.s This should be a bug fix IMO, and be dealt with similarly like ufshcd_is_wb_attrs or ufshcd_is_wb_flag. Thanks, Avri
Hi Avri, On 2020-10-22 14:37, Avri Altman wrote: >> Since WB feature has been added, WB related sysfs entries can be >> accessed >> even when an UFS device does not support WB feature. In that case, the >> descriptors which are not supported by the UFS device may be wrongly >> reported when they are accessed from their corrsponding sysfs entries. >> Fix it by adding a sanity check of parameter offset against the actual >> decriptor length.s > This should be a bug fix IMO, and be dealt with similarly like > ufshcd_is_wb_attrs or ufshcd_is_wb_flag. > Thanks, > Avri Could you please elaborate on ufshcd_is_wb_attrs or ufshcd_is_wb_flag? Sorry that I don't quite get it. Thanks, Can Guo.
> On 2020-10-22 14:37, Avri Altman wrote: > >> Since WB feature has been added, WB related sysfs entries can be > >> accessed > >> even when an UFS device does not support WB feature. In that case, the > >> descriptors which are not supported by the UFS device may be wrongly > >> reported when they are accessed from their corrsponding sysfs entries. > >> Fix it by adding a sanity check of parameter offset against the actual > >> decriptor length.s > > This should be a bug fix IMO, and be dealt with similarly like > > ufshcd_is_wb_attrs or ufshcd_is_wb_flag. > > Thanks, > > Avri > > Could you please elaborate on ufshcd_is_wb_attrs or ufshcd_is_wb_flag? > Sorry that I don't quite get it. Since this change is only protecting illegal access from sysfs entries, I am suggesting to handle it there, just like ufshcd_is_wb_attrs or ufshcd_is_wb_flag Are doing it for flags and attributes. Thanks, Avri
On 2020-10-26 13:22, Avri Altman wrote: >> On 2020-10-22 14:37, Avri Altman wrote: >> >> Since WB feature has been added, WB related sysfs entries can be >> >> accessed >> >> even when an UFS device does not support WB feature. In that case, the >> >> descriptors which are not supported by the UFS device may be wrongly >> >> reported when they are accessed from their corrsponding sysfs entries. >> >> Fix it by adding a sanity check of parameter offset against the actual >> >> decriptor length.s >> > This should be a bug fix IMO, and be dealt with similarly like >> > ufshcd_is_wb_attrs or ufshcd_is_wb_flag. >> > Thanks, >> > Avri >> >> Could you please elaborate on ufshcd_is_wb_attrs or ufshcd_is_wb_flag? >> Sorry that I don't quite get it. > Since this change is only protecting illegal access from sysfs entries, > I am suggesting to handle it there, just like ufshcd_is_wb_attrs or > ufshcd_is_wb_flag > Are doing it for flags and attributes. > > Thanks, > Avri This is a general problem - if later we have HPB entries added into sysfs, we will hit it again. We cannot keep adding checks like ufshcd_is_xxx_attrs or ufshcd_is_xxx_flag to block them, right? Thanks, Can Guo.
On 10/21/2020 10:59 PM, Can Guo wrote: > Since WB feature has been added, WB related sysfs entries can be accessed > even when an UFS device does not support WB feature. In that case, the > descriptors which are not supported by the UFS device may be wrongly > reported when they are accessed from their corrsponding sysfs entries. > Fix it by adding a sanity check of parameter offset against the actual > decriptor length. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- Reviewed-by: Asutosh Das <asutoshd@codeaurora.org> > drivers/scsi/ufs/ufshcd.c | 24 +++++++++++++++--------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index a2ebcc8..aeec10d 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3184,13 +3184,19 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, > /* Get the length of descriptor */ > ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len); > if (!buff_len) { > - dev_err(hba->dev, "%s: Failed to get desc length", __func__); > + dev_err(hba->dev, "%s: Failed to get desc length\n", __func__); > + return -EINVAL; > + } > + > + if (param_offset >= buff_len) { > + dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN 0x%x, length 0x%x\n", > + __func__, param_offset, desc_id, buff_len); > return -EINVAL; > } > > /* Check whether we need temp memory */ > if (param_offset != 0 || param_size < buff_len) { > - desc_buf = kmalloc(buff_len, GFP_KERNEL); > + desc_buf = kzalloc(buff_len, GFP_KERNEL); > if (!desc_buf) > return -ENOMEM; > } else { > @@ -3204,14 +3210,14 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, > desc_buf, &buff_len); > > if (ret) { > - dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d", > + dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d\n", > __func__, desc_id, desc_index, param_offset, ret); > goto out; > } > > /* Sanity check */ > if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id) { > - dev_err(hba->dev, "%s: invalid desc_id %d in descriptor header", > + dev_err(hba->dev, "%s: invalid desc_id %d in descriptor header\n", > __func__, desc_buf[QUERY_DESC_DESC_TYPE_OFFSET]); > ret = -EINVAL; > goto out; > @@ -3221,12 +3227,12 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, > buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET]; > ufshcd_update_desc_length(hba, desc_id, desc_index, buff_len); > > - /* Check wherher we will not copy more data, than available */ > - if (is_kmalloc && (param_offset + param_size) > buff_len) > - param_size = buff_len - param_offset; > - > - if (is_kmalloc) > + 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); > + } > out: > if (is_kmalloc) > kfree(desc_buf); >
On Wed, 21 Oct 2020 22:59:00 -0700, Can Guo wrote: > Since WB feature has been added, WB related sysfs entries can be accessed > even when an UFS device does not support WB feature. In that case, the > descriptors which are not supported by the UFS device may be wrongly > reported when they are accessed from their corrsponding sysfs entries. > Fix it by adding a sanity check of parameter offset against the actual > decriptor length. Applied to 5.10/scsi-fixes, thanks! [1/1] scsi: ufs: Fix unexpected values from ufshcd_read_desc_param() https://git.kernel.org/mkp/scsi/c/1699f980d87f
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index a2ebcc8..aeec10d 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3184,13 +3184,19 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, /* Get the length of descriptor */ ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len); if (!buff_len) { - dev_err(hba->dev, "%s: Failed to get desc length", __func__); + dev_err(hba->dev, "%s: Failed to get desc length\n", __func__); + return -EINVAL; + } + + if (param_offset >= buff_len) { + dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN 0x%x, length 0x%x\n", + __func__, param_offset, desc_id, buff_len); return -EINVAL; } /* Check whether we need temp memory */ if (param_offset != 0 || param_size < buff_len) { - desc_buf = kmalloc(buff_len, GFP_KERNEL); + desc_buf = kzalloc(buff_len, GFP_KERNEL); if (!desc_buf) return -ENOMEM; } else { @@ -3204,14 +3210,14 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, desc_buf, &buff_len); if (ret) { - dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d", + dev_err(hba->dev, "%s: Failed reading descriptor. desc_id %d, desc_index %d, param_offset %d, ret %d\n", __func__, desc_id, desc_index, param_offset, ret); goto out; } /* Sanity check */ if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id) { - dev_err(hba->dev, "%s: invalid desc_id %d in descriptor header", + dev_err(hba->dev, "%s: invalid desc_id %d in descriptor header\n", __func__, desc_buf[QUERY_DESC_DESC_TYPE_OFFSET]); ret = -EINVAL; goto out; @@ -3221,12 +3227,12 @@ int ufshcd_read_desc_param(struct ufs_hba *hba, buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET]; ufshcd_update_desc_length(hba, desc_id, desc_index, buff_len); - /* Check wherher we will not copy more data, than available */ - if (is_kmalloc && (param_offset + param_size) > buff_len) - param_size = buff_len - param_offset; - - if (is_kmalloc) + 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); + } out: if (is_kmalloc) kfree(desc_buf);
Since WB feature has been added, WB related sysfs entries can be accessed even when an UFS device does not support WB feature. In that case, the descriptors which are not supported by the UFS device may be wrongly reported when they are accessed from their corrsponding sysfs entries. Fix it by adding a sanity check of parameter offset against the actual decriptor length. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)