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
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
