[bug report] drm/amd/display: Reuse parsing code of debugfs write buffer

[Dan Carpenter]

Hello Mikita Lipski,

The patch 04111850cf56: "drm/amd/display: Reuse parsing code of
debugfs write buffer" from Mar 26, 2020, leads to the following
static checker warning:

	drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_debugfs.c:80 parse_write_buffer_into_params()
	error: 'copy_from_user()' 'wr_buf_ptr' too small (100 vs 1000000000)

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_debugfs.c
    64  static int parse_write_buffer_into_params(char *wr_buf, uint32_t wr_buf_size,
    65                                            long *param, const char __user *buf,
    66                                            int max_param_num,
    67                                            uint8_t *param_nums)
    68  {
    69          char *wr_buf_ptr = NULL;
    70          uint32_t wr_buf_count = 0;
    71          int r;
    72          char *sub_str = NULL;
    73          const char delimiter[3] = {' ', '\n', '\0'};
    74          uint8_t param_index = 0;
    75  
    76          *param_nums = 0;
    77  
    78          wr_buf_ptr = wr_buf;
    79  
    80          r = copy_from_user(wr_buf_ptr, buf, wr_buf_size);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The "wr_buf_ptr" is a fixed size buffer and the "wr_buf_size" comes from
the user so this would be a privalege escalation bug if it weren't
debugfs.

It's better to move the allocation into this function.  Remember to
free before returning.

		wr_buf = memdup_user(buf, wr_buf_size + 1);
		if (IS_ERR(wr_buf))
			return PTR_ERR(wr_buf);

(Written in email client.  Not compile tested.)

    81  
    82                  /* r is bytes not be copied */
    83          if (r >= wr_buf_size) {

This test is wrong.  It should have been zero instead of wr_buf_size
but that doesn't matter now that we've switched to memdup_user().


    84                  DRM_DEBUG_DRIVER("user data not be read\n");
    85                  return -EINVAL;

Don't print an error if copy_from_user() fails.  Return -EFAULT instead
of -EINVAL.

    86          }
    87  
    88          /* check number of parameters. isspace could not differ space and \n */
    89          while ((*wr_buf_ptr != 0xa) && (wr_buf_count < wr_buf_size)) {
    90                  /* skip space*/
    91                  while (isspace(*wr_buf_ptr) && (wr_buf_count < wr_buf_size)) {
    92                          wr_buf_ptr++;
    93                          wr_buf_count++;
    94                          }
    95  
    96                  if (wr_buf_count == wr_buf_size)
    97                          break;
    98  
    99                  /* skip non-space*/
   100                  while ((!isspace(*wr_buf_ptr)) && (wr_buf_count < wr_buf_size)) {
   101                          wr_buf_ptr++;
   102                          wr_buf_count++;
   103                  }
   104  
   105                  (*param_nums)++;
   106  
   107                  if (wr_buf_count == wr_buf_size)
   108                          break;
   109          }
   110  
   111          if (*param_nums > max_param_num)
   112                  *param_nums = max_param_num;
   113  
   114          wr_buf_ptr = wr_buf; /* reset buf pointer */
   115          wr_buf_count = 0; /* number of char already checked */
   116  
   117          while (isspace(*wr_buf_ptr) && (wr_buf_count < wr_buf_size)) {
   118                  wr_buf_ptr++;
   119                  wr_buf_count++;
   120          }
   121  
   122          while (param_index < *param_nums) {
   123                  /* after strsep, wr_buf_ptr will be moved to after space */
   124                  sub_str = strsep(&wr_buf_ptr, delimiter);

This code is not checking wr_buf_count.  I guess the thinking was that
the first look would find the number of words and the next loop would
use that pre-validated count.  There is not a one to one relationship
between your delimeters and ispace() (tabs) but I guess your delimeter
is a subset of isspace() so that works...

This code also assumes that "wr_buf_ptr" is NUL terminated but that's
not necessarily the case so it could lead to a buffer overflow.  For
example, imagine the whole buffer is full of tabs.  That's counted as
one word.  We're already off the end of the buffer before we call
strsep().  Or if the whole buffer is spaces except for the last
character.  That's again one word and the strsep will read beyond the
end and corrupt memory.

   125  
   126                  r = kstrtol(sub_str, 16, &(param[param_index]));


   127  
   128                  if (r)
   129                          DRM_DEBUG_DRIVER("string to int convert error code: %d\n", r);
   130  
   131                  param_index++;
   132          }
   133  
   134          return 0;
   135  }

This whole function could probably be written like (not compiled):

	wr_buf = memdup_user(buf, wr_buf_size + 1);
	if (IS_ERR(wr_buf))
		return PTR_ERR(wr_buf);
	wr_buf[wr_buf_size] = '\0';

	p = wr_buf;
	while (p && *p) {
		if (*param_nums >= max_param_num)
			goto done;
		while (*p && isspace(*p))
			p++;
		sub_str = strsep(&p, delimiter);
		ret = kstrtol(sub_str, 16, &(param[param_index]));
		if (ret)
			goto done;
		(*param_nums)++;
	}
done:
	kfree(wr_buf);
	return 0;

regards,
dan carpenter