[bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Dan Carpenter]

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

[bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Dan Carpenter]

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

Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Dan Carpenter]

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

RE: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Zhou1, Tao]

[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&amp;data=02%7C01%7Ctao.zhou1%40amd.com%7Cd9925eca763149180
> ea508d7f0d47cfe%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C6
> 37242667754892396&amp;sdata=k%2FMTgz9D1jIJGRFBu2Yuu6wuHP%2BPbR
> vEcNJp87slIJE%3D&amp;reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Dan Carpenter]

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

Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Pan, Xinhui]

[-- 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

Re: [bug report] drm/amdgpu: add amdgpu_ras.c to support ras (v2)

[Dan Carpenter]

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