diff mbox series

scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param()

Message ID 1603189751-26541-1-git-send-email-cang@codeaurora.org (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: Fix unexpected values get from ufshcd_read_desc_param() | expand

Commit Message

Can Guo Oct. 20, 2020, 10:29 a.m. UTC 
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>

Comments

kernel test robot Oct. 20, 2020, 2:52 p.m. UTC | #1 
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
diff mbox series

Patch

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);