[PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Mario Limonciello]

Testing the code that is submitted for kernel 6.6 found some occasional
problems with memory allocation and with the test suite and sample
application as merged.

This series fixes all problems found. Ideally all patches should
be submitted for a future 6.6-rc, but, the most important is
"crypto: ccp: Get a free page to use while fetching initial nonce"
if the rest must be pushed to 6.7.

This series merges together the two previous submissions, re-orders them
and fixes one more problem found with unfused tests.
Link: https://lore.kernel.org/linux-crypto/20230828190200.1598-1-mario.limonciello@amd.com/T/#m5f336a50707d2426c2a0d47d751f06d233e5c816
Link: https://lore.kernel.org/linux-crypto/20230824221932.2807-1-mario.limonciello@amd.com/#t

Mario Limonciello (5):
  crypto: ccp: Get a free page to use while fetching initial nonce
  crypto: ccp: Fix ioctl unit tests
  crypto: ccp: Fix DBC sample application error handling
  crypto: ccp: Fix sample application signature passing
  crypto: ccp: Fix some unfused tests

 drivers/crypto/ccp/dbc.c     |  2 +-
 tools/crypto/ccp/dbc.c       | 17 +++++++-------
 tools/crypto/ccp/dbc.py      |  8 +++----
 tools/crypto/ccp/test_dbc.py | 45 +++++++++++++++++++++---------------
 4 files changed, 41 insertions(+), 31 deletions(-)


base-commit: 85b9bf9a514d991fcecb118d0a8a35e754ff9265
-- 
2.34.1

[PATCH v3 1/5] crypto: ccp: Get a free page to use while fetching initial nonce

[Mario Limonciello]

dbc_dev_init() gets a free page from `GFP_KERNEL`, but if that page has
any data in it the first nonce request will fail.
This prevents dynamic boost control from probing. To fix this, explicitly
request a zeroed page with `__GFP_ZERO` to ensure first nonce fetch works.

Fixes: c04cf9e14f10 ("crypto: ccp - Add support for fetching a nonce for dynamic boost control")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/crypto/ccp/dbc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/ccp/dbc.c b/drivers/crypto/ccp/dbc.c
index 839ea14b9a85..6f33149ef80d 100644
--- a/drivers/crypto/ccp/dbc.c
+++ b/drivers/crypto/ccp/dbc.c
@@ -205,7 +205,7 @@ int dbc_dev_init(struct psp_device *psp)
 		return -ENOMEM;
 
 	BUILD_BUG_ON(sizeof(union dbc_buffer) > PAGE_SIZE);
-	dbc_dev->mbox = (void *)devm_get_free_pages(dev, GFP_KERNEL, 0);
+	dbc_dev->mbox = (void *)devm_get_free_pages(dev, GFP_KERNEL | __GFP_ZERO, 0);
 	if (!dbc_dev->mbox) {
 		ret = -ENOMEM;
 		goto cleanup_dev;
-- 
2.34.1

[PATCH v3 2/5] crypto: ccp: Fix ioctl unit tests

[Mario Limonciello]

A local environment change was importing ioctl_opt which is required
for ioctl tests to pass.  Add the missing import for it.

Fixes: 15f8aa7bb3e5 ("crypto: ccp - Add unit tests for dynamic boost control")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 tools/crypto/ccp/test_dbc.py | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/tools/crypto/ccp/test_dbc.py b/tools/crypto/ccp/test_dbc.py
index 998bb3e3cd04..a28a1f94c1d2 100755
--- a/tools/crypto/ccp/test_dbc.py
+++ b/tools/crypto/ccp/test_dbc.py
@@ -4,6 +4,12 @@ import unittest
 import os
 import time
 import glob
+import fcntl
+try:
+    import ioctl_opt as ioctl
+except ImportError:
+    ioctl = None
+    pass
 from dbc import *
 
 # Artificial delay between set commands
@@ -64,13 +70,16 @@ class TestInvalidIoctls(DynamicBoostControlTest):
     def setUp(self) -> None:
         if not os.path.exists(DEVICE_NODE):
             self.skipTest("system is unsupported")
+        if not ioctl:
+            self.skipTest("unable to test IOCTLs without ioctl_opt")
+
         return super().setUp()
 
     def test_invalid_nonce_ioctl(self) -> None:
         """tries to call get_nonce ioctl with invalid data structures"""
 
         # 0x1 (get nonce), and invalid data
-        INVALID1 = IOWR(ord("D"), 0x01, invalid_param)
+        INVALID1 = ioctl.IOWR(ord("D"), 0x01, invalid_param)
         with self.assertRaises(OSError) as error:
             fcntl.ioctl(self.d, INVALID1, self.data, True)
         self.assertEqual(error.exception.errno, 22)
@@ -79,7 +88,7 @@ class TestInvalidIoctls(DynamicBoostControlTest):
         """tries to call set_uid ioctl with invalid data structures"""
 
         # 0x2 (set uid), and invalid data
-        INVALID2 = IOW(ord("D"), 0x02, invalid_param)
+        INVALID2 = ioctl.IOW(ord("D"), 0x02, invalid_param)
         with self.assertRaises(OSError) as error:
             fcntl.ioctl(self.d, INVALID2, self.data, True)
         self.assertEqual(error.exception.errno, 22)
@@ -88,7 +97,7 @@ class TestInvalidIoctls(DynamicBoostControlTest):
         """tries to call set_uid ioctl with invalid data structures"""
 
         # 0x2 as RW (set uid), and invalid data
-        INVALID3 = IOWR(ord("D"), 0x02, invalid_param)
+        INVALID3 = ioctl.IOWR(ord("D"), 0x02, invalid_param)
         with self.assertRaises(OSError) as error:
             fcntl.ioctl(self.d, INVALID3, self.data, True)
         self.assertEqual(error.exception.errno, 22)
@@ -96,7 +105,7 @@ class TestInvalidIoctls(DynamicBoostControlTest):
     def test_invalid_param_ioctl(self) -> None:
         """tries to call param ioctl with invalid data structures"""
         # 0x3 (param), and invalid data
-        INVALID4 = IOWR(ord("D"), 0x03, invalid_param)
+        INVALID4 = ioctl.IOWR(ord("D"), 0x03, invalid_param)
         with self.assertRaises(OSError) as error:
             fcntl.ioctl(self.d, INVALID4, self.data, True)
         self.assertEqual(error.exception.errno, 22)
@@ -104,7 +113,7 @@ class TestInvalidIoctls(DynamicBoostControlTest):
     def test_invalid_call_ioctl(self) -> None:
         """tries to call the DBC ioctl with invalid data structures"""
         # 0x4, and invalid data
-        INVALID5 = IOWR(ord("D"), 0x04, invalid_param)
+        INVALID5 = ioctl.IOWR(ord("D"), 0x04, invalid_param)
         with self.assertRaises(OSError) as error:
             fcntl.ioctl(self.d, INVALID5, self.data, True)
         self.assertEqual(error.exception.errno, 22)
-- 
2.34.1

[PATCH v3 3/5] crypto: ccp: Fix DBC sample application error handling

[Mario Limonciello]

The sample application was taking values from ioctl() and treating
those as the error codes to present to a user.

This is incorrect when ret is non-zero, the error is stored to `errno`.
Use this value instead.

Fixes: f40d42f116cf ("crypto: ccp - Add a sample python script for Dynamic Boost Control")
Fixes: febe3ed3222f ("crypto: ccp - Add a sample library for ioctl use")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 tools/crypto/ccp/dbc.c  | 16 ++++++++--------
 tools/crypto/ccp/dbc.py |  3 +--
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/tools/crypto/ccp/dbc.c b/tools/crypto/ccp/dbc.c
index 37e813175642..7774e981849f 100644
--- a/tools/crypto/ccp/dbc.c
+++ b/tools/crypto/ccp/dbc.c
@@ -8,6 +8,7 @@
  */
 
 #include <assert.h>
+#include <errno.h>
 #include <string.h>
 #include <sys/ioctl.h>
 
@@ -22,16 +23,14 @@ int get_nonce(int fd, void *nonce_out, void *signature)
 	struct dbc_user_nonce tmp = {
 		.auth_needed = !!signature,
 	};
-	int ret;
 
 	assert(nonce_out);
 
 	if (signature)
 		memcpy(tmp.signature, signature, sizeof(tmp.signature));
 
-	ret = ioctl(fd, DBCIOCNONCE, &tmp);
-	if (ret)
-		return ret;
+	if (ioctl(fd, DBCIOCNONCE, &tmp))
+		return errno;
 	memcpy(nonce_out, tmp.nonce, sizeof(tmp.nonce));
 
 	return 0;
@@ -47,7 +46,9 @@ int set_uid(int fd, __u8 *uid, __u8 *signature)
 	memcpy(tmp.uid, uid, sizeof(tmp.uid));
 	memcpy(tmp.signature, signature, sizeof(tmp.signature));
 
-	return ioctl(fd, DBCIOCUID, &tmp);
+	if (ioctl(fd, DBCIOCUID, &tmp))
+		return errno;
+	return 0;
 }
 
 int process_param(int fd, int msg_index, __u8 *signature, int *data)
@@ -63,9 +64,8 @@ int process_param(int fd, int msg_index, __u8 *signature, int *data)
 
 	memcpy(tmp.signature, signature, sizeof(tmp.signature));
 
-	ret = ioctl(fd, DBCIOCPARAM, &tmp);
-	if (ret)
-		return ret;
+	if (ioctl(fd, DBCIOCPARAM, &tmp))
+		return errno;
 
 	*data = tmp.param;
 	return 0;
diff --git a/tools/crypto/ccp/dbc.py b/tools/crypto/ccp/dbc.py
index 3f6a825ffc9e..3956efe7537a 100644
--- a/tools/crypto/ccp/dbc.py
+++ b/tools/crypto/ccp/dbc.py
@@ -27,8 +27,7 @@ lib = ctypes.CDLL("./dbc_library.so", mode=ctypes.RTLD_GLOBAL)
 
 
 def handle_error(code):
-    val = code * -1
-    raise OSError(val, os.strerror(val))
+    raise OSError(code, os.strerror(code))
 
 
 def get_nonce(device, signature):
-- 
2.34.1

[PATCH v3 4/5] crypto: ccp: Fix sample application signature passing

[Mario Limonciello]

When parameters are sent the PSP returns back it's own signature
for the application to verify the authenticity of the result.

Display this signature to the caller instead of the one the caller
sent.

Fixes: f40d42f116cf ("crypto: ccp - Add a sample python script for Dynamic Boost Control")
Fixes: febe3ed3222f ("crypto: ccp - Add a sample library for ioctl use")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 tools/crypto/ccp/dbc.c  | 1 +
 tools/crypto/ccp/dbc.py | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/crypto/ccp/dbc.c b/tools/crypto/ccp/dbc.c
index 7774e981849f..a807df0f0597 100644
--- a/tools/crypto/ccp/dbc.c
+++ b/tools/crypto/ccp/dbc.c
@@ -68,5 +68,6 @@ int process_param(int fd, int msg_index, __u8 *signature, int *data)
 		return errno;
 
 	*data = tmp.param;
+	memcpy(signature, tmp.signature, sizeof(tmp.signature));
 	return 0;
 }
diff --git a/tools/crypto/ccp/dbc.py b/tools/crypto/ccp/dbc.py
index 3956efe7537a..2b91415b1940 100644
--- a/tools/crypto/ccp/dbc.py
+++ b/tools/crypto/ccp/dbc.py
@@ -57,7 +57,8 @@ def process_param(device, message, signature, data=None):
     if type(message) != tuple:
         raise ValueError("Expected message tuple")
     arg = ctypes.c_int(data if data else 0)
-    ret = lib.process_param(device.fileno(), message[0], signature, ctypes.pointer(arg))
+    sig = ctypes.create_string_buffer(signature, len(signature))
+    ret = lib.process_param(device.fileno(), message[0], ctypes.pointer(sig), ctypes.pointer(arg))
     if ret:
         handle_error(ret)
-    return arg, signature
+    return arg.value, sig.value
-- 
2.34.1

[PATCH v3 5/5] crypto: ccp: Fix some unfused tests

[Mario Limonciello]

Some of the tests for unfused parts referenced a named member parameter,
but when the test suite was switched to call a python ctypes library they
weren't updated.  Adjust them to refer to the first argument of the
process_param() call and set the data type of the signature appropriately.

Fixes: 15f8aa7bb3e5 ("crypto: ccp - Add unit tests for dynamic boost control")
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 tools/crypto/ccp/test_dbc.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/tools/crypto/ccp/test_dbc.py b/tools/crypto/ccp/test_dbc.py
index a28a1f94c1d2..79de3638a01a 100755
--- a/tools/crypto/ccp/test_dbc.py
+++ b/tools/crypto/ccp/test_dbc.py
@@ -33,8 +33,8 @@ def system_is_secured() -> bool:
 class DynamicBoostControlTest(unittest.TestCase):
     def __init__(self, data) -> None:
         self.d = None
-        self.signature = "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"
-        self.uid = "1111111111111111"
+        self.signature = b"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"
+        self.uid = b"1111111111111111"
         super().__init__(data)
 
     def setUp(self) -> None:
@@ -192,12 +192,12 @@ class TestUnFusedSystem(DynamicBoostControlTest):
         # SOC power
         soc_power_max = process_param(self.d, PARAM_GET_SOC_PWR_MAX, self.signature)
         soc_power_min = process_param(self.d, PARAM_GET_SOC_PWR_MIN, self.signature)
-        self.assertGreater(soc_power_max.parameter, soc_power_min.parameter)
+        self.assertGreater(soc_power_max[0], soc_power_min[0])
 
         # fmax
         fmax_max = process_param(self.d, PARAM_GET_FMAX_MAX, self.signature)
         fmax_min = process_param(self.d, PARAM_GET_FMAX_MIN, self.signature)
-        self.assertGreater(fmax_max.parameter, fmax_min.parameter)
+        self.assertGreater(fmax_max[0], fmax_min[0])
 
         # cap values
         keys = {
@@ -208,7 +208,7 @@ class TestUnFusedSystem(DynamicBoostControlTest):
         }
         for k in keys:
             result = process_param(self.d, keys[k], self.signature)
-            self.assertGreater(result.parameter, 0)
+            self.assertGreater(result[0], 0)
 
     def test_get_invalid_param(self) -> None:
         """fetch an invalid parameter"""
@@ -226,17 +226,17 @@ class TestUnFusedSystem(DynamicBoostControlTest):
         original = process_param(self.d, PARAM_GET_FMAX_CAP, self.signature)
 
         # set the fmax
-        target = original.parameter - 100
+        target = original[0] - 100
         process_param(self.d, PARAM_SET_FMAX_CAP, self.signature, target)
         time.sleep(SET_DELAY)
         new = process_param(self.d, PARAM_GET_FMAX_CAP, self.signature)
-        self.assertEqual(new.parameter, target)
+        self.assertEqual(new[0], target)
 
         # revert back to current
-        process_param(self.d, PARAM_SET_FMAX_CAP, self.signature, original.parameter)
+        process_param(self.d, PARAM_SET_FMAX_CAP, self.signature, original[0])
         time.sleep(SET_DELAY)
         cur = process_param(self.d, PARAM_GET_FMAX_CAP, self.signature)
-        self.assertEqual(cur.parameter, original.parameter)
+        self.assertEqual(cur[0], original[0])
 
     def test_set_power_cap(self) -> None:
         """get/set power cap limit"""
@@ -244,17 +244,17 @@ class TestUnFusedSystem(DynamicBoostControlTest):
         original = process_param(self.d, PARAM_GET_PWR_CAP, self.signature)
 
         # set the fmax
-        target = original.parameter - 10
+        target = original[0] - 10
         process_param(self.d, PARAM_SET_PWR_CAP, self.signature, target)
         time.sleep(SET_DELAY)
         new = process_param(self.d, PARAM_GET_PWR_CAP, self.signature)
-        self.assertEqual(new.parameter, target)
+        self.assertEqual(new[0], target)
 
         # revert back to current
-        process_param(self.d, PARAM_SET_PWR_CAP, self.signature, original.parameter)
+        process_param(self.d, PARAM_SET_PWR_CAP, self.signature, original[0])
         time.sleep(SET_DELAY)
         cur = process_param(self.d, PARAM_GET_PWR_CAP, self.signature)
-        self.assertEqual(cur.parameter, original.parameter)
+        self.assertEqual(cur[0], original[0])
 
     def test_set_3d_graphics_mode(self) -> None:
         """set/get 3d graphics mode"""
-- 
2.34.1

Re: [PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Herbert Xu]

On Tue, Aug 29, 2023 at 10:07:54AM -0500, Mario Limonciello wrote:
>
> This series fixes all problems found. Ideally all patches should
> be submitted for a future 6.6-rc, but, the most important is
> "crypto: ccp: Get a free page to use while fetching initial nonce"
> if the rest must be pushed to 6.7.

As far as I can see it simply means that the new feature of DBC does
not work.  If that's all then I don't think it's an urgent issue and
this can wait until 6.7.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Mario Limonciello]

On 8/31/2023 04:02, Herbert Xu wrote:
> On Tue, Aug 29, 2023 at 10:07:54AM -0500, Mario Limonciello wrote:
>>
>> This series fixes all problems found. Ideally all patches should
>> be submitted for a future 6.6-rc, but, the most important is
>> "crypto: ccp: Get a free page to use while fetching initial nonce"
>> if the rest must be pushed to 6.7.
> 
> As far as I can see it simply means that the new feature of DBC does
> not work.  If that's all then I don't think it's an urgent issue and
> this can wait until 6.7.
> 
> Cheers,

It works; but it's a failure rate type issue based on what is in the 
page it gets.  I didn't reproduce it when it was initially submitted.
I only noticed it when testing from Linus' tree.
So I suspect it's related to other completely unrelated code that now
runs before ccp is loaded by the kernel.

So some boots you see the character device and can interact with it, 
others you can't.  If you start with 'ccp' loaded and it working, then 
unload/reload it might not come back.

Re: [PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Herbert Xu]

On Thu, Aug 31, 2023 at 06:27:59AM -0500, Mario Limonciello wrote:
>
> So some boots you see the character device and can interact with it, others
> you can't.  If you start with 'ccp' loaded and it working, then
> unload/reload it might not come back.

Is it just the new functionality that's broken or does it cause
existing functionality to fail as well?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Mario Limonciello]

On 8/31/2023 06:56, Herbert Xu wrote:
> On Thu, Aug 31, 2023 at 06:27:59AM -0500, Mario Limonciello wrote:
>>
>> So some boots you see the character device and can interact with it, others
>> you can't.  If you start with 'ccp' loaded and it working, then
>> unload/reload it might not come back.
> 
> Is it just the new functionality that's broken or does it cause
> existing functionality to fail as well?
> 
> Thanks,

Fortunately it's non-fatal for any other functionality in the driver.

Re: [PATCH v3 0/5] Fixes for dynamic boost control for 6.6

[Herbert Xu]

On Tue, Aug 29, 2023 at 10:07:54AM -0500, Mario Limonciello wrote:
> Testing the code that is submitted for kernel 6.6 found some occasional
> problems with memory allocation and with the test suite and sample
> application as merged.
> 
> This series fixes all problems found. Ideally all patches should
> be submitted for a future 6.6-rc, but, the most important is
> "crypto: ccp: Get a free page to use while fetching initial nonce"
> if the rest must be pushed to 6.7.
> 
> This series merges together the two previous submissions, re-orders them
> and fixes one more problem found with unfused tests.
> Link: https://lore.kernel.org/linux-crypto/20230828190200.1598-1-mario.limonciello@amd.com/T/#m5f336a50707d2426c2a0d47d751f06d233e5c816
> Link: https://lore.kernel.org/linux-crypto/20230824221932.2807-1-mario.limonciello@amd.com/#t
> 
> Mario Limonciello (5):
>   crypto: ccp: Get a free page to use while fetching initial nonce
>   crypto: ccp: Fix ioctl unit tests
>   crypto: ccp: Fix DBC sample application error handling
>   crypto: ccp: Fix sample application signature passing
>   crypto: ccp: Fix some unfused tests
> 
>  drivers/crypto/ccp/dbc.c     |  2 +-
>  tools/crypto/ccp/dbc.c       | 17 +++++++-------
>  tools/crypto/ccp/dbc.py      |  8 +++----
>  tools/crypto/ccp/test_dbc.py | 45 +++++++++++++++++++++---------------
>  4 files changed, 41 insertions(+), 31 deletions(-)
> 
> 
> base-commit: 85b9bf9a514d991fcecb118d0a8a35e754ff9265
> -- 
> 2.34.1

All applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt