diff mbox series

[v4] dt-bindings: dvfs: Add support for generic performance domains

Message ID 20210517155458.1016707-1-sudeep.holla@arm.com
State Accepted
Commit 88bf5a85fe9840c9b49c5f6c625cdccd11233943
Headers show
Series [v4] dt-bindings: dvfs: Add support for generic performance domains | expand

Commit Message

Sudeep Holla May 17, 2021, 3:54 p.m. UTC
The CLKSCREW attack [0] exposed security vulnerabilities in energy management
implementations where untrusted software had direct access to clock and
voltage hardware controls. In this attack, the malicious software was able to
place the platform into unsafe overclocked or undervolted configurations. Such
configurations then enabled the injection of predictable faults to reveal
secrets.

Many Arm-based systems used to or still use voltage regulator and clock
frameworks in the kernel. These frameworks allow callers to independently
manipulate frequency and voltage settings. Such implementations can render
systems susceptible to this form of attack.

Attacks such as CLKSCREW are now being mitigated by not having direct and
independent control of clock and voltage in the kernel and moving that
control to a trusted entity, such as the SCP firmware or secure world
firmware/software which are to perform sanity checking on the requested
performance levels, thereby preventing any attempted malicious programming.

With the advent of such an abstraction, there is a need to replace the
generic clock and regulator bindings used by such devices with a generic
performance domains bindings.

[0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang

Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
Cc: Rob Herring <robh+dt@kernel.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

---
Hi All,

Sorry for yet another delay, I don't want to mist this for v5.14 as Mediatek
cpufreq driver was depending on this IIRC.

v3[3]->v4:
	- Dropped unnecessary phandle-array reference
	- Added maxItems = 1 for the property

v2[2]->v3[3]:
	- Dropped required properties
	- Added non cpu device example
	- Updated cpu bindings too

v1[1]->v2[2]:
	- Changed to Dual License
	- Added select: true, enum for #performance-domain-cells and
	  $ref for performance-domain
	- Changed the example to use real existing compatibles instead
	  of made-up ones

Regards,
Sudeep

[1] https://lore.kernel.org/r/20201105173539.1426301-1-sudeep.holla@arm.com
[2] https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com
[3] https://lore.kernel.org/r/20210407135913.2067694-1-sudeep.holla@arm.com

 .../devicetree/bindings/arm/cpus.yaml         |  7 ++
 .../bindings/dvfs/performance-domain.yaml     | 74 +++++++++++++++++++
 2 files changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml

--
2.25.1

Comments

Rob Herring May 17, 2021, 7:17 p.m. UTC | #1
On Mon, May 17, 2021 at 10:55 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>

> The CLKSCREW attack [0] exposed security vulnerabilities in energy management

> implementations where untrusted software had direct access to clock and

> voltage hardware controls. In this attack, the malicious software was able to

> place the platform into unsafe overclocked or undervolted configurations. Such

> configurations then enabled the injection of predictable faults to reveal

> secrets.

>

> Many Arm-based systems used to or still use voltage regulator and clock

> frameworks in the kernel. These frameworks allow callers to independently

> manipulate frequency and voltage settings. Such implementations can render

> systems susceptible to this form of attack.

>

> Attacks such as CLKSCREW are now being mitigated by not having direct and

> independent control of clock and voltage in the kernel and moving that

> control to a trusted entity, such as the SCP firmware or secure world

> firmware/software which are to perform sanity checking on the requested

> performance levels, thereby preventing any attempted malicious programming.

>

> With the advent of such an abstraction, there is a need to replace the

> generic clock and regulator bindings used by such devices with a generic

> performance domains bindings.

>

> [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang

>

> Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com

> Cc: Rob Herring <robh+dt@kernel.org>

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>


Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring May 17, 2021, 8:45 p.m. UTC | #2
On Mon, 17 May 2021 16:54:58 +0100, Sudeep Holla wrote:
> The CLKSCREW attack [0] exposed security vulnerabilities in energy management

> implementations where untrusted software had direct access to clock and

> voltage hardware controls. In this attack, the malicious software was able to

> place the platform into unsafe overclocked or undervolted configurations. Such

> configurations then enabled the injection of predictable faults to reveal

> secrets.

> 

> Many Arm-based systems used to or still use voltage regulator and clock

> frameworks in the kernel. These frameworks allow callers to independently

> manipulate frequency and voltage settings. Such implementations can render

> systems susceptible to this form of attack.

> 

> Attacks such as CLKSCREW are now being mitigated by not having direct and

> independent control of clock and voltage in the kernel and moving that

> control to a trusted entity, such as the SCP firmware or secure world

> firmware/software which are to perform sanity checking on the requested

> performance levels, thereby preventing any attempted malicious programming.

> 

> With the advent of such an abstraction, there is a need to replace the

> generic clock and regulator bindings used by such devices with a generic

> performance domains bindings.

> 

> [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang

> 

> Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com

> Cc: Rob Herring <robh+dt@kernel.org>

> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

> Hi All,

> 

> Sorry for yet another delay, I don't want to mist this for v5.14 as Mediatek

> cpufreq driver was depending on this IIRC.

> 

> v3[3]->v4:

> 	- Dropped unnecessary phandle-array reference

> 	- Added maxItems = 1 for the property

> 

> v2[2]->v3[3]:

> 	- Dropped required properties

> 	- Added non cpu device example

> 	- Updated cpu bindings too

> 

> v1[1]->v2[2]:

> 	- Changed to Dual License

> 	- Added select: true, enum for #performance-domain-cells and

> 	  $ref for performance-domain

> 	- Changed the example to use real existing compatibles instead

> 	  of made-up ones

> 

> Regards,

> Sudeep

> 

> [1] https://lore.kernel.org/r/20201105173539.1426301-1-sudeep.holla@arm.com

> [2] https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com

> [3] https://lore.kernel.org/r/20210407135913.2067694-1-sudeep.holla@arm.com

> 

>  .../devicetree/bindings/arm/cpus.yaml         |  7 ++

>  .../bindings/dvfs/performance-domain.yaml     | 74 +++++++++++++++++++

>  2 files changed, 81 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml

> 


My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']

See https://patchwork.ozlabs.org/patch/1479615

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Sudeep Holla May 19, 2021, 11:20 a.m. UTC | #3
Hi Rob,

On Mon, May 17, 2021 at 03:45:11PM -0500, Rob Herring wrote:

[...]

> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'

> on your patch (DT_CHECKER_FLAGS is new in v5.13):

>

> yamllint warnings/errors:

>

> dtschema/dtc warnings/errors:

> Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']

>

> See https://patchwork.ozlabs.org/patch/1479615


IIUC, such errors due to the fact that the compatible used in the example
is not in any yaml schema(as it is still in the old txt format). I also
assume such errors are allowed until the transition is complete and I
need not fix anything as part of this patch ?

--
Regards,
Sudeep
Sudeep Holla May 19, 2021, 11:23 a.m. UTC | #4
On Mon, May 17, 2021 at 02:17:05PM -0500, Rob Herring wrote:
> On Mon, May 17, 2021 at 10:55 AM Sudeep Holla <sudeep.holla@arm.com> wrote:

> >

> > The CLKSCREW attack [0] exposed security vulnerabilities in energy management

> > implementations where untrusted software had direct access to clock and

> > voltage hardware controls. In this attack, the malicious software was able to

> > place the platform into unsafe overclocked or undervolted configurations. Such

> > configurations then enabled the injection of predictable faults to reveal

> > secrets.

> >

> > Many Arm-based systems used to or still use voltage regulator and clock

> > frameworks in the kernel. These frameworks allow callers to independently

> > manipulate frequency and voltage settings. Such implementations can render

> > systems susceptible to this form of attack.

> >

> > Attacks such as CLKSCREW are now being mitigated by not having direct and

> > independent control of clock and voltage in the kernel and moving that

> > control to a trusted entity, such as the SCP firmware or secure world

> > firmware/software which are to perform sanity checking on the requested

> > performance levels, thereby preventing any attempted malicious programming.

> >

> > With the advent of such an abstraction, there is a need to replace the

> > generic clock and regulator bindings used by such devices with a generic

> > performance domains bindings.

> >

> > [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang

> >

> > Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.holla@arm.com

> > Cc: Rob Herring <robh+dt@kernel.org>

> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

> 

> Reviewed-by: Rob Herring <robh@kernel.org>


Thanks Rob.

Viresh,

I noticed I haven't cc-ed linux-pm list, do you need me to post there or
are you happy to pick it up from here when Hector's mediatek cpufreq drivers
using this are ready to be merged ?

--
Regards,
Sudeep
Viresh Kumar May 20, 2021, 3:54 a.m. UTC | #5
On 19-05-21, 12:23, Sudeep Holla wrote:
> I noticed I haven't cc-ed linux-pm list, do you need me to post there or

> are you happy to pick it up from here when Hector's mediatek cpufreq drivers

> using this are ready to be merged ?


Applied. Thanks.

-- 
viresh
Rob Herring May 20, 2021, 7:43 p.m. UTC | #6
On Wed, May 19, 2021 at 6:20 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>

> Hi Rob,

>

> On Mon, May 17, 2021 at 03:45:11PM -0500, Rob Herring wrote:

>

> [...]

>

> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'

> > on your patch (DT_CHECKER_FLAGS is new in v5.13):

> >

> > yamllint warnings/errors:

> >

> > dtschema/dtc warnings/errors:

> > Documentation/devicetree/bindings/dvfs/performance-domain.example.dt.yaml:0:0: /example-0/performance-controller@12340000: failed to match any schema with compatible: ['qcom,cpufreq-hw']

> >

> > See https://patchwork.ozlabs.org/patch/1479615

>

> IIUC, such errors due to the fact that the compatible used in the example

> is not in any yaml schema(as it is still in the old txt format). I also

> assume such errors are allowed until the transition is complete and I

> need not fix anything as part of this patch ?


Not allowed because I can't turn this check on by default until we get
rid of the existing 80 or so. But it is a new check and Viresh already
applied, so oh well.

BTW, one of the 80 is 'operating-points-v2' compatible. Hint, hint.

Rob
Viresh Kumar May 21, 2021, 4:08 a.m. UTC | #7
On 20-05-21, 14:43, Rob Herring wrote:
> Not allowed because I can't turn this check on by default until we get

> rid of the existing 80 or so. But it is a new check and Viresh already

> applied, so oh well.


I can always drop it :)

-- 
viresh
Sudeep Holla May 21, 2021, 3:24 p.m. UTC | #8
On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:
> On 20-05-21, 14:43, Rob Herring wrote:

> > Not allowed because I can't turn this check on by default until we get

> > rid of the existing 80 or so. But it is a new check and Viresh already

> > applied, so oh well.

>

> I can always drop it :)

>


While I really don't care(evident by rate at which I worked on this 😉)
I think Hector Yuan won't be happy to wait I guess. As a quick fix, you
can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still
get warning with this patch + update alone, but once you have Hector's
mediatek cpufreq driver applied, the warnings must disappear.

What do you think ?

--
Regards,
Sudeep
Viresh Kumar May 24, 2021, 9:17 a.m. UTC | #9
On 21-05-21, 16:24, Sudeep Holla wrote:
> On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:

> > On 20-05-21, 14:43, Rob Herring wrote:

> > > Not allowed because I can't turn this check on by default until we get

> > > rid of the existing 80 or so. But it is a new check and Viresh already

> > > applied, so oh well.

> >

> > I can always drop it :)

> >

> 

> While I really don't care(evident by rate at which I worked on this 😉)

> I think Hector Yuan won't be happy to wait I guess. As a quick fix, you

> can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still

> get warning with this patch + update alone, but once you have Hector's

> mediatek cpufreq driver applied, the warnings must disappear.

> 

> What do you think ?


I guess you can send a fix later then.

-- 
viresh
Sudeep Holla May 24, 2021, 10:05 a.m. UTC | #10
On Mon, May 24, 2021 at 02:47:15PM +0530, Viresh Kumar wrote:
> On 21-05-21, 16:24, Sudeep Holla wrote:

> > On Fri, May 21, 2021 at 09:38:34AM +0530, Viresh Kumar wrote:

> > > On 20-05-21, 14:43, Rob Herring wrote:

> > > > Not allowed because I can't turn this check on by default until we get

> > > > rid of the existing 80 or so. But it is a new check and Viresh already

> > > > applied, so oh well.

> > >

> > > I can always drop it :)

> > >

> > 

> > While I really don't care(evident by rate at which I worked on this 😉)

> > I think Hector Yuan won't be happy to wait I guess. As a quick fix, you

> > can update "qcom,cpufreq-hw" to "mediatek,cpufreq-hw". You will still

> > get warning with this patch + update alone, but once you have Hector's

> > mediatek cpufreq driver applied, the warnings must disappear.

> > 

> > What do you think ?

> 

> I guess you can send a fix later then.


Sure 😄

-- 
Regards,
Sudeep
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml
index f3c7249c73d6..9a2432a88074 100644
--- a/Documentation/devicetree/bindings/arm/cpus.yaml
+++ b/Documentation/devicetree/bindings/arm/cpus.yaml
@@ -257,6 +257,13 @@  description: |+

       where voltage is in V, frequency is in MHz.

+  performance-domains:
+    maxItems: 1
+    description:
+      List of phandles and performance domain specifiers, as defined by
+      bindings of the performance domain provider. See also
+      dvfs/performance-domain.yaml.
+
   power-domains:
     description:
       List of phandles and PM domain specifiers, as defined by bindings of the
diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
new file mode 100644
index 000000000000..c8b91207f34d
--- /dev/null
+++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
@@ -0,0 +1,74 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic performance domains
+
+maintainers:
+  - Sudeep Holla <sudeep.holla@arm.com>
+
+description: |+
+  This binding is intended for performance management of groups of devices or
+  CPUs that run in the same performance domain. Performance domains must not
+  be confused with power domains. A performance domain is defined by a set
+  of devices that always have to run at the same performance level. For a given
+  performance domain, there is a single point of control that affects all the
+  devices in the domain, making it impossible to set the performance level of
+  an individual device in the domain independently from other devices in
+  that domain. For example, a set of CPUs that share a voltage domain, and
+  have a common frequency control, is said to be in the same performance
+  domain.
+
+  This device tree binding can be used to bind performance domain consumer
+  devices with their performance domains provided by performance domain
+  providers. A performance domain provider can be represented by any node in
+  the device tree and can provide one or more performance domains. A consumer
+  node can refer to the provider by a phandle and a set of phandle arguments
+  (so called performance domain specifiers) of length specified by the
+  \#performance-domain-cells property in the performance domain provider node.
+
+select: true
+
+properties:
+  "#performance-domain-cells":
+    description:
+      Number of cells in a performance domain specifier. Typically 0 for nodes
+      representing a single performance domain and 1 for nodes providing
+      multiple performance domains (e.g. performance controllers), but can be
+      any value as specified by device tree binding documentation of particular
+      provider.
+    enum: [ 0, 1 ]
+
+  performance-domains:
+    $ref: '/schemas/types.yaml#/definitions/phandle-array'
+    maxItems: 1
+    description:
+      A phandle and performance domain specifier as defined by bindings of the
+      performance controller/provider specified by phandle.
+
+additionalProperties: true
+
+examples:
+  - |
+    performance: performance-controller@12340000 {
+        compatible = "qcom,cpufreq-hw";
+        reg = <0x12340000 0x1000>;
+        #performance-domain-cells = <1>;
+    };
+
+    // The node above defines a performance controller that is a performance
+    // domain provider and expects one cell as its phandle argument.
+
+    cpus {
+        #address-cells = <2>;
+        #size-cells = <0>;
+
+        cpu@0 {
+            device_type = "cpu";
+            compatible = "arm,cortex-a57";
+            reg = <0x0 0x0>;
+            performance-domains = <&performance 1>;
+        };
+    };