* [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param()
@ 2020-10-20 10:29 Can Guo
2020-10-20 14:52 ` kernel test robot
0 siblings, 1 reply; 3+ messages in thread
From: Can Guo @ 2020-10-20 10:29 UTC (permalink / raw)
To: asutoshd, nguyenb, hongwus, rnayak, linux-scsi, kernel-team,
saravanak, salyzyn, cang
Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bean Huo, Bart Van Assche,
open list
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>
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a2ebcc8..8861ad6 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);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param()
2020-10-20 10:29 [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param() Can Guo
@ 2020-10-20 14:52 ` kernel test robot
0 siblings, 0 replies; 3+ messages in thread
From: kernel test robot @ 2020-10-20 14:52 UTC (permalink / raw)
To: Can Guo, asutoshd, nguyenb, hongwus, rnayak, linux-scsi,
kernel-team, saravanak, salyzyn
Cc: kbuild-all, Alim Akhtar
[-- Attachment #1: Type: text/plain, Size: 12949 bytes --]
Hi Can,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on scsi/for-next]
[also build test ERROR on mkp-scsi/for-next v5.9 next-20201016]
[cannot apply to target/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Can-Guo/scsi-ufs-Fix-unexpected-values-get-from-ufshcd_read_desc_param/20201020-183121
base: https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: openrisc-randconfig-r026-20201020 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/63b44a6aaa719b0d2eb2ed982279c9dc38fabb30
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Can-Guo/scsi-ufs-Fix-unexpected-values-get-from-ufshcd_read_desc_param/20201020-183121
git checkout 63b44a6aaa719b0d2eb2ed982279c9dc38fabb30
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=openrisc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/scsi/ufs/ufshcd.c: In function 'ufshcd_read_desc_param':
drivers/scsi/ufs/ufshcd.c:3175:7: warning: unused variable 'is_kmalloc' [-Wunused-variable]
3175 | bool is_kmalloc = true;
| ^~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3173:6: warning: unused variable 'desc_buf' [-Wunused-variable]
3173 | u8 *desc_buf;
| ^~~~~~~~
drivers/scsi/ufs/ufshcd.c:3172:6: warning: unused variable 'ret' [-Wunused-variable]
3172 | int ret;
| ^~~
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/async.h:12,
from drivers/scsi/ufs/ufshcd.c:12:
drivers/scsi/ufs/ufshcd.c: At top level:
>> include/linux/compiler.h:56:23: error: expected identifier or '(' before 'if'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~
drivers/scsi/ufs/ufshcd.c:3195:2: note: in expansion of macro 'if'
3195 | if (param_offset != 0 || param_size < buff_len) {
| ^~
>> include/linux/compiler.h:72:2: error: expected identifier or '(' before ')' token
72 | })
| ^
include/linux/compiler.h:58:69: note: in expansion of macro '__trace_if_value'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3195:2: note: in expansion of macro 'if'
3195 | if (param_offset != 0 || param_size < buff_len) {
| ^~
drivers/scsi/ufs/ufshcd.c:3199:4: error: expected identifier or '(' before 'else'
3199 | } else {
| ^~~~
drivers/scsi/ufs/ufshcd.c:3205:2: warning: data definition has no type or storage class
3205 | ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
| ^~~
drivers/scsi/ufs/ufshcd.c:3205:2: error: type defaults to 'int' in declaration of 'ret' [-Werror=implicit-int]
drivers/scsi/ufs/ufshcd.c:3205:38: error: 'hba' undeclared here (not in a function)
3205 | ret = ufshcd_query_descriptor_retry(hba, UPIU_QUERY_OPCODE_READ_DESC,
| ^~~
drivers/scsi/ufs/ufshcd.c:3206:6: error: 'desc_id' undeclared here (not in a function); did you mean 'desc_idn'?
3206 | desc_id, desc_index, 0,
| ^~~~~~~
| desc_idn
drivers/scsi/ufs/ufshcd.c:3206:15: error: 'desc_index' undeclared here (not in a function); did you mean 'desc_idn'?
3206 | desc_id, desc_index, 0,
| ^~~~~~~~~~
| desc_idn
drivers/scsi/ufs/ufshcd.c:3207:6: error: 'desc_buf' undeclared here (not in a function)
3207 | desc_buf, &buff_len);
| ^~~~~~~~
drivers/scsi/ufs/ufshcd.c:3207:17: error: 'buff_len' undeclared here (not in a function)
3207 | desc_buf, &buff_len);
| ^~~~~~~~
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/async.h:12,
from drivers/scsi/ufs/ufshcd.c:12:
>> include/linux/compiler.h:56:23: error: expected identifier or '(' before 'if'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~
drivers/scsi/ufs/ufshcd.c:3209:2: note: in expansion of macro 'if'
3209 | if (ret) {
| ^~
>> include/linux/compiler.h:72:2: error: expected identifier or '(' before ')' token
72 | })
| ^
include/linux/compiler.h:58:69: note: in expansion of macro '__trace_if_value'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3209:2: note: in expansion of macro 'if'
3209 | if (ret) {
| ^~
>> include/linux/compiler.h:56:23: error: expected identifier or '(' before 'if'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~
drivers/scsi/ufs/ufshcd.c:3216:2: note: in expansion of macro 'if'
3216 | if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id) {
| ^~
>> include/linux/compiler.h:72:2: error: expected identifier or '(' before ')' token
72 | })
| ^
include/linux/compiler.h:58:69: note: in expansion of macro '__trace_if_value'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3216:2: note: in expansion of macro 'if'
3216 | if (desc_buf[QUERY_DESC_DESC_TYPE_OFFSET] != desc_id) {
| ^~
drivers/scsi/ufs/ufshcd.c:3224:2: warning: data definition has no type or storage class
3224 | buff_len = desc_buf[QUERY_DESC_LENGTH_OFFSET];
| ^~~~~~~~
drivers/scsi/ufs/ufshcd.c:3224:2: error: type defaults to 'int' in declaration of 'buff_len' [-Werror=implicit-int]
drivers/scsi/ufs/ufshcd.c:3225:2: warning: data definition has no type or storage class
3225 | ufshcd_update_desc_length(hba, desc_id, desc_index, buff_len);
| ^~~~~~~~~~~~~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3225:2: error: type defaults to 'int' in declaration of 'ufshcd_update_desc_length' [-Werror=implicit-int]
drivers/scsi/ufs/ufshcd.c:3225:2: warning: parameter names (without types) in function declaration
drivers/scsi/ufs/ufshcd.c:3225:2: error: conflicting types for 'ufshcd_update_desc_length'
drivers/scsi/ufs/ufshcd.c:3140:13: note: previous definition of 'ufshcd_update_desc_length' was here
3140 | static void ufshcd_update_desc_length(struct ufs_hba *hba,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/async.h:12,
from drivers/scsi/ufs/ufshcd.c:12:
>> include/linux/compiler.h:56:23: error: expected identifier or '(' before 'if'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~
drivers/scsi/ufs/ufshcd.c:3227:2: note: in expansion of macro 'if'
3227 | if (is_kmalloc) {
| ^~
>> include/linux/compiler.h:72:2: error: expected identifier or '(' before ')' token
72 | })
| ^
include/linux/compiler.h:58:69: note: in expansion of macro '__trace_if_value'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3227:2: note: in expansion of macro 'if'
3227 | if (is_kmalloc) {
| ^~
drivers/scsi/ufs/ufshcd.c:3233:4: error: expected '=', ',', ';', 'asm' or '__attribute__' before ':' token
3233 | out:
| ^
In file included from include/linux/kernel.h:11,
from include/linux/list.h:9,
from include/linux/async.h:12,
from drivers/scsi/ufs/ufshcd.c:12:
>> include/linux/compiler.h:72:2: error: expected identifier or '(' before ')' token
72 | })
| ^
include/linux/compiler.h:58:69: note: in expansion of macro '__trace_if_value'
58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
| ^~~~~~~~~~~~~~~~
include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
| ^~~~~~~~~~~~~~
drivers/scsi/ufs/ufshcd.c:3234:2: note: in expansion of macro 'if'
3234 | if (is_kmalloc)
| ^~
drivers/scsi/ufs/ufshcd.c:3236:2: error: expected identifier or '(' before 'return'
3236 | return ret;
| ^~~~~~
drivers/scsi/ufs/ufshcd.c:3237:1: error: expected identifier or '(' before '}' token
3237 | }
| ^
drivers/scsi/ufs/ufshcd.c:3140:13: warning: 'ufshcd_update_desc_length' defined but not used [-Wunused-function]
3140 | static void ufshcd_update_desc_length(struct ufs_hba *hba,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +56 include/linux/compiler.h
2bcd521a684cc94 Steven Rostedt 2008-11-21 50
2bcd521a684cc94 Steven Rostedt 2008-11-21 51 #ifdef CONFIG_PROFILE_ALL_BRANCHES
2bcd521a684cc94 Steven Rostedt 2008-11-21 52 /*
2bcd521a684cc94 Steven Rostedt 2008-11-21 53 * "Define 'is'", Bill Clinton
2bcd521a684cc94 Steven Rostedt 2008-11-21 54 * "Define 'if'", Steven Rostedt
2bcd521a684cc94 Steven Rostedt 2008-11-21 55 */
a15fd609ad53a63 Linus Torvalds 2019-03-20 @56 #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
a15fd609ad53a63 Linus Torvalds 2019-03-20 57
a15fd609ad53a63 Linus Torvalds 2019-03-20 58 #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
a15fd609ad53a63 Linus Torvalds 2019-03-20 59
a15fd609ad53a63 Linus Torvalds 2019-03-20 60 #define __trace_if_value(cond) ({ \
2bcd521a684cc94 Steven Rostedt 2008-11-21 61 static struct ftrace_branch_data \
e04462fb82f8dd9 Miguel Ojeda 2018-09-03 62 __aligned(4) \
bfafddd8de426d8 Nick Desaulniers 2019-08-28 63 __section(_ftrace_branch) \
a15fd609ad53a63 Linus Torvalds 2019-03-20 64 __if_trace = { \
2bcd521a684cc94 Steven Rostedt 2008-11-21 65 .func = __func__, \
2bcd521a684cc94 Steven Rostedt 2008-11-21 66 .file = __FILE__, \
2bcd521a684cc94 Steven Rostedt 2008-11-21 67 .line = __LINE__, \
2bcd521a684cc94 Steven Rostedt 2008-11-21 68 }; \
a15fd609ad53a63 Linus Torvalds 2019-03-20 69 (cond) ? \
a15fd609ad53a63 Linus Torvalds 2019-03-20 70 (__if_trace.miss_hit[1]++,1) : \
a15fd609ad53a63 Linus Torvalds 2019-03-20 71 (__if_trace.miss_hit[0]++,0); \
a15fd609ad53a63 Linus Torvalds 2019-03-20 @72 })
a15fd609ad53a63 Linus Torvalds 2019-03-20 73
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28086 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param()
@ 2020-10-21 10:28 Bean Huo (beanhuo)
0 siblings, 0 replies; 3+ messages in thread
From: Bean Huo (beanhuo) @ 2020-10-21 10:28 UTC (permalink / raw)
To: Can Guo, asutoshd@codeaurora.org, nguyenb@codeaurora.org,
hongwus@codeaurora.org, rnayak@codeaurora.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
saravanak@google.com, salyzyn@google.com
Cc: Alim Akhtar, Avri Altman, James E.J. Bottomley,
Martin K. Petersen, Stanley Chu, Bart Van Assche, open list
Can,
> 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>
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> a2ebcc8..8861ad6 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;
> }
A brace missed! This right brace misses a left brace.
Bean
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-21 10:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-20 10:29 [PATCH] scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param() Can Guo
2020-10-20 14:52 ` kernel test robot
-- strict thread matches above, loose matches on Subject: below --
2020-10-21 10:28 Bean Huo (beanhuo)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).