* [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
@ 2020-05-05 9:12 Dan Carpenter
2020-05-05 12:37 ` Dan Carpenter
2020-05-06 7:26 ` Zhou1, Tao
0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2020-05-05 9:12 UTC (permalink / raw)
To: xinhui.pan; +Cc: amd-gfx
Hello xinhui pan,
The patch c030f2e4166c: "drm/amdgpu: add amdgpu_ras.c to support ras
(v2)" from Oct 31, 2018, leads to the following static checker
warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:620 amdgpu_ras_feature_enable()
warn: uncapped user index 'ras_block_string[head->block]'
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
587 int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
588 struct ras_common_if *head, bool enable)
589 {
590 struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
591 union ta_ras_cmd_input info;
592 int ret;
593
594 if (!con)
595 return -EINVAL;
596
597 if (!enable) {
598 info.disable_features = (struct ta_ras_disable_features_input) {
599 .block_id = amdgpu_ras_block_to_ta(head->block),
600 .error_type = amdgpu_ras_error_to_ta(head->type),
601 };
602 } else {
603 info.enable_features = (struct ta_ras_enable_features_input) {
604 .block_id = amdgpu_ras_block_to_ta(head->block),
605 .error_type = amdgpu_ras_error_to_ta(head->type),
606 };
607 }
608
609 /* Do not enable if it is not allowed. */
610 WARN_ON(enable && !amdgpu_ras_is_feature_allowed(adev, head));
611 /* Are we alerady in that state we are going to set? */
612 if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
613 return 0;
614
615 if (!amdgpu_ras_intr_triggered()) {
616 ret = psp_ras_enable_features(&adev->psp, &info, enable);
617 if (ret) {
618 amdgpu_ras_parse_status_code(adev,
619 enable ? "enable":"disable",
620 ras_block_str(head->block),
^^^^^^^^^^^
The head->block value can be set to anything using debugfs. That's a
problem because it could easily lead to a kernel panic (which is
annoying) and also because these days we try to restrict what root can
do.
621 (enum ta_ras_status)ret);
622 if (ret == TA_RAS_STATUS__RESET_NEEDED)
623 return -EAGAIN;
624 return -EINVAL;
625 }
626 }
627
628 /* setup the obj */
629 __amdgpu_ras_feature_enable(adev, head, enable);
630
631 return 0;
632 }
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
2020-05-05 9:12 [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2) Dan Carpenter
@ 2020-05-05 12:37 ` Dan Carpenter
2020-05-06 7:26 ` Zhou1, Tao
1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2020-05-05 12:37 UTC (permalink / raw)
To: xinhui.pan; +Cc: amd-gfx
Here are a couple more:
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:534 amdgpu_ras_is_feature_allowed() error: undefined (user controlled) shift '(((1))) << (head->block)'
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:542 amdgpu_ras_is_feature_enabled() error: undefined (user controlled) shift '(((1))) << (head->block)'
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
2020-05-05 9:12 [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2) Dan Carpenter
2020-05-05 12:37 ` Dan Carpenter
@ 2020-05-06 7:26 ` Zhou1, Tao
2020-05-06 9:17 ` Dan Carpenter
1 sibling, 1 reply; 7+ messages in thread
From: Zhou1, Tao @ 2020-05-06 7:26 UTC (permalink / raw)
To: Dan Carpenter, Pan, Xinhui; +Cc: amd-gfx
[AMD Public Use]
Hi Dan:
Please check the following piece of code in amdgpu_ras_debugfs_ctrl_parse_data:
if (op != -1) {
if (amdgpu_ras_find_block_id_by_name(block_name, &block_id))
return -EINVAL;
data->head.block = block_id;
amdgpu_ras_find_block_id_by_name will return error directly if someone try to provide an invalid block_name intentionally via debugfs.
Regards,
Tao
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Dan
> Carpenter
> Sent: 2020年5月5日 17:13
> To: Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> Subject: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
>
> Hello xinhui pan,
>
> The patch c030f2e4166c: "drm/amdgpu: add amdgpu_ras.c to support ras
> (v2)" from Oct 31, 2018, leads to the following static checker
> warning:
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:620
> amdgpu_ras_feature_enable()
> warn: uncapped user index 'ras_block_string[head->block]'
>
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> 587 int amdgpu_ras_feature_enable(struct amdgpu_device *adev,
> 588 struct ras_common_if *head, bool enable)
> 589 {
> 590 struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> 591 union ta_ras_cmd_input info;
> 592 int ret;
> 593
> 594 if (!con)
> 595 return -EINVAL;
> 596
> 597 if (!enable) {
> 598 info.disable_features = (struct ta_ras_disable_features_input)
> {
> 599 .block_id = amdgpu_ras_block_to_ta(head->block),
> 600 .error_type = amdgpu_ras_error_to_ta(head->type),
> 601 };
> 602 } else {
> 603 info.enable_features = (struct ta_ras_enable_features_input)
> {
> 604 .block_id = amdgpu_ras_block_to_ta(head->block),
> 605 .error_type = amdgpu_ras_error_to_ta(head->type),
> 606 };
> 607 }
> 608
> 609 /* Do not enable if it is not allowed. */
> 610 WARN_ON(enable && !amdgpu_ras_is_feature_allowed(adev,
> head));
> 611 /* Are we alerady in that state we are going to set? */
> 612 if (!(!!enable ^ !!amdgpu_ras_is_feature_enabled(adev, head)))
> 613 return 0;
> 614
> 615 if (!amdgpu_ras_intr_triggered()) {
> 616 ret = psp_ras_enable_features(&adev->psp, &info, enable);
> 617 if (ret) {
> 618 amdgpu_ras_parse_status_code(adev,
> 619 enable ? "enable":"disable",
> 620 ras_block_str(head->block),
> ^^^^^^^^^^^ The head->block
> value can be set to anything using debugfs. That's a problem because it
> could easily lead to a kernel panic (which is
> annoying) and also because these days we try to restrict what root can do.
>
> 621 (enum ta_ras_status)ret);
> 622 if (ret == TA_RAS_STATUS__RESET_NEEDED)
> 623 return -EAGAIN;
> 624 return -EINVAL;
> 625 }
> 626 }
> 627
> 628 /* setup the obj */
> 629 __amdgpu_ras_feature_enable(adev, head, enable);
> 630
> 631 return 0;
> 632 }
>
> regards,
> dan carpenter
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.f
> reedesktop.org%2Fmailman%2Flistinfo%2Famd-
> gfx&data=02%7C01%7Ctao.zhou1%40amd.com%7Cd9925eca763149180
> ea508d7f0d47cfe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37242667754892396&sdata=k%2FMTgz9D1jIJGRFBu2Yuu6wuHP%2BPbR
> vEcNJp87slIJE%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
2020-05-06 7:26 ` Zhou1, Tao
@ 2020-05-06 9:17 ` Dan Carpenter
2020-05-06 10:10 ` Pan, Xinhui
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2020-05-06 9:17 UTC (permalink / raw)
To: Zhou1, Tao; +Cc: Pan, Xinhui, amd-gfx
On Wed, May 06, 2020 at 07:26:16AM +0000, Zhou1, Tao wrote:
> [AMD Public Use]
>
> Hi Dan:
>
> Please check the following piece of code in amdgpu_ras_debugfs_ctrl_parse_data:
>
> if (op != -1) {
> if (amdgpu_ras_find_block_id_by_name(block_name, &block_id))
> return -EINVAL;
>
> data->head.block = block_id;
>
> amdgpu_ras_find_block_id_by_name will return error directly if someone try to provide an invalid block_name intentionally via debugfs.
>
No. It's the line after that which are the problem.
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
147 static int amdgpu_ras_debugfs_ctrl_parse_data(struct file *f,
148 const char __user *buf, size_t size,
149 loff_t *pos, struct ras_debug_if *data)
150 {
151 ssize_t s = min_t(u64, 64, size);
152 char str[65];
153 char block_name[33];
154 char err[9] = "ue";
155 int op = -1;
156 int block_id;
157 uint32_t sub_block;
158 u64 address, value;
159
160 if (*pos)
161 return -EINVAL;
162 *pos = size;
163
164 memset(str, 0, sizeof(str));
165 memset(data, 0, sizeof(*data));
166
167 if (copy_from_user(str, buf, s))
168 return -EINVAL;
169
170 if (sscanf(str, "disable %32s", block_name) == 1)
171 op = 0;
172 else if (sscanf(str, "enable %32s %8s", block_name, err) == 2)
173 op = 1;
174 else if (sscanf(str, "inject %32s %8s", block_name, err) == 2)
175 op = 2;
176 else if (str[0] && str[1] && str[2] && str[3])
177 /* ascii string, but commands are not matched. */
Say we don't write an ascii string.
178 return -EINVAL;
179
180 if (op != -1) {
181 if (amdgpu_ras_find_block_id_by_name(block_name, &block_id))
182 return -EINVAL;
183
184 data->head.block = block_id;
185 /* only ue and ce errors are supported */
186 if (!memcmp("ue", err, 2))
187 data->head.type = AMDGPU_RAS_ERROR__MULTI_UNCORRECTABLE;
188 else if (!memcmp("ce", err, 2))
189 data->head.type = AMDGPU_RAS_ERROR__SINGLE_CORRECTABLE;
190 else
191 return -EINVAL;
192
193 data->op = op;
194
195 if (op == 2) {
196 if (sscanf(str, "%*s %*s %*s %u %llu %llu",
197 &sub_block, &address, &value) != 3)
198 if (sscanf(str, "%*s %*s %*s 0x%x 0x%llx 0x%llx",
199 &sub_block, &address, &value) != 3)
200 return -EINVAL;
201 data->head.sub_block_index = sub_block;
202 data->inject.address = address;
203 data->inject.value = value;
204 }
205 } else {
206 if (size < sizeof(*data))
207 return -EINVAL;
208
209 if (copy_from_user(data, buf, sizeof(*data)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This lets us set the data->head.block to whatever we want. Premusably
there is a trusted app which knows how to write the correct values.
But if it has a bug that will cause a crash and we'll have to find a
way to disable it in the kernel for kernel lock down mode etc so either
way we'll need to do a bit of work.
210 return -EINVAL;
211 }
212
213 return 0;
214 }
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
2020-05-06 9:17 ` Dan Carpenter
@ 2020-05-06 10:10 ` Pan, Xinhui
2020-05-06 11:33 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Pan, Xinhui @ 2020-05-06 10:10 UTC (permalink / raw)
To: Zhou1, Tao, Dan Carpenter; +Cc: amd-gfx
[-- Attachment #1.1: Type: text/plain, Size: 4837 bytes --]
[AMD Official Use Only - Internal Distribution Only]
no. below function checks if block is valid or not.
I think you need check your code_checker. or you were checking on a very old codebase?
/* check if ras is supported on block, say, sdma, gfx */
static inline int amdgpu_ras_is_supported(struct amdgpu_device *adev,
unsigned int block)
________________________________
From: Dan Carpenter <dan.carpenter@oracle.com>
Sent: Wednesday, May 6, 2020 5:17:34 PM
To: Zhou1, Tao <Tao.Zhou1@amd.com>
Cc: Pan, Xinhui <Xinhui.Pan@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
On Wed, May 06, 2020 at 07:26:16AM +0000, Zhou1, Tao wrote:
> [AMD Public Use]
>
> Hi Dan:
>
> Please check the following piece of code in amdgpu_ras_debugfs_ctrl_parse_data:
>
> if (op != -1) {
> if (amdgpu_ras_find_block_id_by_name(block_name, &block_id))
> return -EINVAL;
>
> data->head.block = block_id;
>
> amdgpu_ras_find_block_id_by_name will return error directly if someone try to provide an invalid block_name intentionally via debugfs.
>
No. It's the line after that which are the problem.
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
147 static int amdgpu_ras_debugfs_ctrl_parse_data(struct file *f,
148 const char __user *buf, size_t size,
149 loff_t *pos, struct ras_debug_if *data)
150 {
151 ssize_t s = min_t(u64, 64, size);
152 char str[65];
153 char block_name[33];
154 char err[9] = "ue";
155 int op = -1;
156 int block_id;
157 uint32_t sub_block;
158 u64 address, value;
159
160 if (*pos)
161 return -EINVAL;
162 *pos = size;
163
164 memset(str, 0, sizeof(str));
165 memset(data, 0, sizeof(*data));
166
167 if (copy_from_user(str, buf, s))
168 return -EINVAL;
169
170 if (sscanf(str, "disable %32s", block_name) == 1)
171 op = 0;
172 else if (sscanf(str, "enable %32s %8s", block_name, err) == 2)
173 op = 1;
174 else if (sscanf(str, "inject %32s %8s", block_name, err) == 2)
175 op = 2;
176 else if (str[0] && str[1] && str[2] && str[3])
177 /* ascii string, but commands are not matched. */
Say we don't write an ascii string.
178 return -EINVAL;
179
180 if (op != -1) {
181 if (amdgpu_ras_find_block_id_by_name(block_name, &block_id))
182 return -EINVAL;
183
184 data->head.block = block_id;
185 /* only ue and ce errors are supported */
186 if (!memcmp("ue", err, 2))
187 data->head.type = AMDGPU_RAS_ERROR__MULTI_UNCORRECTABLE;
188 else if (!memcmp("ce", err, 2))
189 data->head.type = AMDGPU_RAS_ERROR__SINGLE_CORRECTABLE;
190 else
191 return -EINVAL;
192
193 data->op = op;
194
195 if (op == 2) {
196 if (sscanf(str, "%*s %*s %*s %u %llu %llu",
197 &sub_block, &address, &value) != 3)
198 if (sscanf(str, "%*s %*s %*s 0x%x 0x%llx 0x%llx",
199 &sub_block, &address, &value) != 3)
200 return -EINVAL;
201 data->head.sub_block_index = sub_block;
202 data->inject.address = address;
203 data->inject.value = value;
204 }
205 } else {
206 if (size < sizeof(*data))
207 return -EINVAL;
208
209 if (copy_from_user(data, buf, sizeof(*data)))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This lets us set the data->head.block to whatever we want. Premusably
there is a trusted app which knows how to write the correct values.
But if it has a bug that will cause a crash and we'll have to find a
way to disable it in the kernel for kernel lock down mode etc so either
way we'll need to do a bit of work.
210 return -EINVAL;
211 }
212
213 return 0;
214 }
regards,
dan carpenter
[-- Attachment #1.2: Type: text/html, Size: 12745 bytes --]
[-- Attachment #2: Type: text/plain, Size: 154 bytes --]
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
2020-05-06 10:10 ` Pan, Xinhui
@ 2020-05-06 11:33 ` Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2020-05-06 11:33 UTC (permalink / raw)
To: Pan, Xinhui; +Cc: Zhou1, Tao, amd-gfx
On Wed, May 06, 2020 at 10:10:56AM +0000, Pan, Xinhui wrote:
> [AMD Official Use Only - Internal Distribution Only]
>
> no. below function checks if block is valid or not.
> I think you need check your code_checker. or you were checking on a very old codebase?
>
> /* check if ras is supported on block, say, sdma, gfx */
> static inline int amdgpu_ras_is_supported(struct amdgpu_device *adev,
> unsigned int block)
Ah! That's right. Thanks.
What happens here is that Smatch thinks amdgpu_ras_is_supported() always
returns false because there is a bug in how it tracks ras->supported.
I will fix this.
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)
@ 2019-03-20 13:23 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-03-20 13:23 UTC (permalink / raw)
To: xinhui.pan-5C7GfCeVMHo; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
Hello xinhui pan,
This is a semi-automatic email about new static checker warnings.
The patch dbd249c24427: "drm/amdgpu: add amdgpu_ras.c to support ras
(v2)" from Oct 31, 2018, leads to the following Smatch complaint:
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1215 amdgpu_ras_add_bad_pages()
warn: variable dereferenced before check 'con' (see line 1211)
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1247 amdgpu_ras_reserve_bad_pages()
warn: variable dereferenced before check 'con' (see line 1242)
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
1210 struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
1211 struct ras_err_handler_data *data = con->eh_data;
^^^^^^^^^^^^
Dereference
1212 int i = pages;
1213 int ret = 0;
1214
1215 if (!con || !data || !bps || pages <= 0)
^^^^
Check
1216 return 0;
1217
regards,
dan carpenter
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-06 11:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 9:12 [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2) Dan Carpenter
2020-05-05 12:37 ` Dan Carpenter
2020-05-06 7:26 ` Zhou1, Tao
2020-05-06 9:17 ` Dan Carpenter
2020-05-06 10:10 ` Pan, Xinhui
2020-05-06 11:33 ` Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2019-03-20 13:23 Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.