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