RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode

["Quan, Evan" <Evan.Quan@xxxxxxx>]

[AMD Official Use Only]

 

 

 

From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Nikolic, Marina
Sent: Tuesday, December 21, 2021 10:36 PM
To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Mitrovic, Milan <Milan.Mitrovic@xxxxxxx>; Kitchen, Greg <Greg.Kitchen@xxxxxxx>
Subject: Re: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode

 

[AMD Official Use Only]

 

[AMD Official Use Only]

 

From 06359f3be0c0b889519d6dd954fb11f31e9a15e0 Mon Sep 17 00:00:00 2001

From: Marina Nikolic <Marina.Nikolic@xxxxxxx>

Date: Tue, 14 Dec 2021 20:57:53 +0800

Subject: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read

 permission in ONEVF mode

[Quan, Evan] With the subject updated(remove the description about pp_dpm_sclk), the patch is acked-by: Evan Quan <evan.quan@xxxxxxx>

 

BR

Evan

== Description ==

Setting through sysfs should not be allowed in SRIOV mode.

These calls will not be processed by FW anyway,

but error handling on sysfs level should be improved.

 

== Changes ==

This patch prohibits performing of all set commands

in SRIOV mode on sysfs level.

It offers better error handling as calls that are

not allowed will not be propagated further.

 

== Test ==

Writing to any sysfs file in passthrough mode will succeed.

Writing to any sysfs file in ONEVF mode will yield error:

"calling process does not have sufficient permission to execute a command".

 

Signed-off-by: Marina Nikolic <Marina.Nikolic@xxxxxxx>

---

 drivers/gpu/drm/amd/pm/amdgpu_pm.c | 6 ++++++

 1 file changed, 6 insertions(+)

 

diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c b/drivers/gpu/drm/amd/pm/amdgpu_pm.c

index 082539c70fd4..c43818cd02aa 100644

--- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c

+++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c

@@ -2133,6 +2133,12 @@ static int default_attr_update(struct amdgpu_device *adev, struct amdgpu_device_

                }

        }

 

+       /* setting should not be allowed from VF */

+       if (amdgpu_sriov_vf(adev)) {

+               dev_attr->attr.mode &= ~S_IWUGO;

+               dev_attr->store = NULL;

+       }

+

 #undef DEVICE_ATTR_IS

 

        return 0;

--

2.20.1

 

---

From: Nikolic, Marina <Marina.Nikolic@xxxxxxx>
Sent: Tuesday, December 21, 2021 3:15 PM
To: Russell, Kent <Kent.Russell@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Mitrovic, Milan <Milan.Mitrovic@xxxxxxx>; Kitchen, Greg <Greg.Kitchen@xxxxxxx>
Subject: Re: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode

 

Hi Kent, 

 

Thank you for the review. Yes, I can confirm I am trying to set this for every single file for SRIOV mode.

@Kitchen, Greg required this for ROCM-SMI 5.0 release. In case you need it, he can provide more details.

I'm going to clarify commit message more and send a new patch.

 

BR,
Marina

---

From: Russell, Kent <Kent.Russell@xxxxxxx>
Sent: Monday, December 20, 2021 8:01 PM
To: Nikolic, Marina <Marina.Nikolic@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
Cc: Mitrovic, Milan <Milan.Mitrovic@xxxxxxx>; Nikolic, Marina <Marina.Nikolic@xxxxxxx>; Kitchen, Greg <Greg.Kitchen@xxxxxxx>
Subject: RE: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in ONEVF mode

 

[AMD Official Use Only]

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Marina Nikolic
> Sent: Monday, December 20, 2021 11:09 AM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Mitrovic, Milan <Milan.Mitrovic@xxxxxxx>; Nikolic, Marina
> <Marina.Nikolic@xxxxxxx>; Kitchen, Greg <Greg.Kitchen@xxxxxxx>
> Subject: [PATCH] amdgpu/pm: Modify sysfs pp_dpm_sclk to have only read premission in
> ONEVF mode
>
> == Description ==
> Due to security reasons setting through sysfs
> should only be allowed in passthrough mode.
> Options that are not mapped as SMU messages
> do not have any mechanizm to distinguish between
> passthorugh, onevf and mutivf usecase.
> A unified approach is needed.
>
> == Changes ==
> This patch introduces a new mechanizm to distinguish
> ONEVF and PASSTHROUGH use case on sysfs level
> and prohibit setting (writting to sysfs).
> It also applies the new mechanizm on pp_dpm_sclk sysfs file.
>
> == Test ==
> Writing to pp_dpm_sclk sysfs file in passthrough mode will succeed.
> Writing to pp_dpm_sclk sysfs file in ONEVF mode will yield error:
> "calling process does not have sufficient permission to execute a command".
> Sysfs pp_dpm_sclk will not be created in MULTIVF mode.
>
> Signed-off-by: Marina Nikolic <Marina.Nikolic@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/pm/amdgpu_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> index 082539c70fd4..d2b168babc7d 100644
> --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> @@ -2133,6 +2133,12 @@ static int default_attr_update(struct amdgpu_device *adev,
> struct amdgpu_device_
>               }
>       }
>
> +     /* security: setting should not be allowed from VF */
> +     if (amdgpu_sriov_vf(adev)) {

You should be checking for pp_dpm_sclk here, for example:
                if (DEVICE_ATTR_IS(pp_dpm_sclk) {

Otherwise I am pretty sure you're setting this for every single file. And is it only sclk? Or does it also need to affect mclk/fclk/etc? If it's only sclk, the line above should help. If it's for more, then the commit should try to clarify that as it's not 100% clear.

 Kent

> +             dev_attr->attr.mode &= ~S_IWUGO;
> +             dev_attr->store = NULL;
> +     }
> +
>  #undef DEVICE_ATTR_IS
>
>       return 0;
> --
> 2.20.1