Add a Rockchip secure boot PTA #7661

tretter · 2025-12-17T16:33:09Z

This merge request adds a PTA for enabling secure boot on Rockchip SoCs.

On Rockchip SoCs, secure boot must be enabled from the secure world. There are other solutions to enable secure boot very early from a bootloader or special application. This has the drawback that user interaction or scripting on the device is not possible, which is inconvenient during development and software deployment in the factory.

This PTA allows enabling secure boot for Rockchip devices with an application running in the normal world.

There are still a few open issues in the PTA. I'd like to get some feedback on this approach:

Is this PTA the right approach for enabling secure boot on such devices? Are other approaches (TF-A?) better suited or already established?
Is the API of the PTA is reasonable and useful? Is there anything I have to change to bring it in line with OP-TEE standards?
Is the PTA acceptable for upstream OP-TEE as an in-tree PTA?

Function wise, the PTA API provides 3 functions, which work as follows:

Ask the TA from the non-secure world about the current status and hash
of the hardware. This allows to inspect the current status of secure
boot on a specific device.
Write an RSA hash into the OTP fuses. It's the responsibility of the
user to calculate the hash and ensure that it matches the key, which
will be used to sign the images.
Actually lockdown the device by enabling secure boot. This is a
separate step to allow the user to verify the setup before
potentially bricking a device.

After that, the ROM code on the device will only accept images that are signed with the RSA key that matches the hash that has been burned into the OTP area.

Continuation of #7511

tretter · 2025-12-17T21:40:04Z

core/pta/rockchip/rk_secure_boot.c

+	res = rockchip_otp_read_secure(tmp,
+				       ROCKCHIP_OTP_RSA_HASH_INDEX,
+				       ROCKCHIP_OTP_RSA_HASH_SIZE);


The style checker complains about the unaligned parameters, but fixing it would lead to complaints about the line length. What's the OP-TEE way to fix such conflicts?

It's not a problem at this line. I usually structure the code in a way that the problem doesn't occur. Pleasing the checker is good, but what matters is that the code should be easy to read.

I guess you're referring to the problem in write_key_size() above. One way is to use local variables holding the values of ROCKCHIP_OTP_SECURE_BOOT_STATUS_INDEX and ROCKCHIP_OTP_SECURE_BOOT_STATUS_SIZE.

Ah, yes, I was referring to ROCKCHIP_OTP_SECURE_BOOT_STATUS_INDEX and ROCKCHIP_OTP_SECURE_BOOT_STATUS_SIZE. I played with local helper variables, but it made the code less readable.

lib/libutee/include/pta_rk_secure_boot.h

tretter · 2025-12-17T21:46:49Z

core/pta/rockchip/rk_secure_boot.c

+static TEE_Result otp_to_string(uint32_t *otp, size_t otp_size,
+				char *str, size_t str_size)
+{
+	if (otp_size != 8 || str_size != HASH_STRING_SIZE)
+		return TEE_ERROR_BAD_PARAMETERS;
+
+	snprintf(str, str_size,
+		 "0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x",
+		 otp[0], otp[1], otp[2], otp[3],
+		 otp[4], otp[5], otp[6], otp[7]);
+
+	return TEE_SUCCESS;
+}


I wanted to be able to decide the log level in the caller. Thus, I implemented this as a format helper, but it's still clumsy to use. Unfortunately, I didn't find any better helpers in OP-TEE. Did I miss something obvious, or is there a better way to solve this?

You never check the return value from otp_to_string() and the you're always supposed to use a proper otp hash size as input. How about simplifying it to allow it to be called in an argument to the *MSG() macro like:

static char *otp_to_string(uint32_t *otp, char *str, size_t str_size) { snprintf(str, str_size, ...); return str; }

Another option (perhaps even better) is to improve the DHEXDUMP() macro with versions for I and E.

I locally added IHEXDUMP() and EHEXDUMP(), but I think that the log output isn't that readable, since the dump and the actual message are in different lines.

I changed the function to return str which makes the use in the prints much nicer. Thanks!

lib/libutee/include/pta_rk_secure_boot.h

jenswi-linaro · 2025-12-18T08:29:07Z

core/pta/rockchip/rk_secure_boot.c

+	res = rockchip_otp_read_secure(tmp,
+				       ROCKCHIP_OTP_RSA_HASH_INDEX,
+				       ROCKCHIP_OTP_RSA_HASH_SIZE);


It's not a problem at this line. I usually structure the code in a way that the problem doesn't occur. Pleasing the checker is good, but what matters is that the code should be easy to read.

I guess you're referring to the problem in write_key_size() above. One way is to use local variables holding the values of ROCKCHIP_OTP_SECURE_BOOT_STATUS_INDEX and ROCKCHIP_OTP_SECURE_BOOT_STATUS_SIZE.

jenswi-linaro · 2025-12-18T08:41:50Z

core/pta/rockchip/rk_secure_boot.c

+static TEE_Result otp_to_string(uint32_t *otp, size_t otp_size,
+				char *str, size_t str_size)
+{
+	if (otp_size != 8 || str_size != HASH_STRING_SIZE)
+		return TEE_ERROR_BAD_PARAMETERS;
+
+	snprintf(str, str_size,
+		 "0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x",
+		 otp[0], otp[1], otp[2], otp[3],
+		 otp[4], otp[5], otp[6], otp[7]);
+
+	return TEE_SUCCESS;
+}


You never check the return value from otp_to_string() and the you're always supposed to use a proper otp hash size as input. How about simplifying it to allow it to be called in an argument to the *MSG() macro like:

static char *otp_to_string(uint32_t *otp, char *str, size_t str_size) { snprintf(str, str_size, ...); return str; }

Another option (perhaps even better) is to improve the DHEXDUMP() macro with versions for I and E.

jenswi-linaro · 2025-12-18T08:45:31Z

core/pta/rockchip/rk_secure_boot.c

+		return TEE_ERROR_BAD_PARAMETERS;
+
+	snprintf(str, str_size,
+		 "0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x 0x%08x",


Please use PRIx32 instead of "x".

I changed all places that used %x to PRIx32.

jenswi-linaro · 2025-12-18T08:53:49Z

core/pta/rockchip/rk_secure_boot.c

+{
+	size_t i;
+
+	if (!a || !b)


This check isn't needed.

I dropped the entire function and replaced it with memcmp.

jenswi-linaro · 2025-12-18T08:55:02Z

core/pta/rockchip/rk_secure_boot.c

+}
+
+/* Test if the two passed public root key hashes are equal. */
+static TEE_Result hashcmp(uint32_t *a, uint32_t *b, size_t s)


Why is this function needed? It seems memcmp() should do the job.

In a previous iteration, I returned the length of the matching values, which would allow continuing writing a partially written hash into the OTP. Eventually, I dropped this feature, but kept the hashcmp function around.

I replaced it with memcmp() now.

jenswi-linaro · 2025-12-18T08:57:46Z

core/pta/rockchip/rk_secure_boot.c

+ */
+static const int simulation = IS_ENABLED2(CFG_RK_SECURE_BOOT_SIMULATION);
+
+static inline bool bit_test(uint32_t value, uint32_t bit)


This function name is a potential conflict with a macro with the same name in <bitstring.h>.

Would it be helpful to have this as a generic function with a useful name somewhere in the utils?

The name of this function should be test_bits() or test_bit_mask() to better reflect what it's supposed to do. The function is too odd to be generically usable.

jenswi-linaro · 2025-12-18T08:58:44Z

core/pta/rockchip/rk_secure_boot.c

+ * option is necessary to actually enable secure boot with PTA, but may
+ * potentially brick your device.
+ */
+static const int simulation = IS_ENABLED2(CFG_RK_SECURE_BOOT_SIMULATION);


Why not use IS_ENABLED(CFG_RK_SECURE_BOOT_SIMULATION) directly in the code?

With this construction, I have a single place (instead of 3 occurrences in the code) for documentation. I also thought about adding a command to enable or disable the simulation, but eventually decided that I want this to be a compile time instead of a runtime option.

CFG variables are better documented in the .mk file where they are introduced. Please use the IS_ENABLED() (yes, the version without the 2) macro directly instead.

I understand. I moved the documentation to the .mk and dropped the simulation variable.

jenswi-linaro · 2025-12-18T09:09:31Z

core/pta/rockchip/rk_secure_boot.c

+	if (bytes_size * sizeof(*bytes) != otp_size * sizeof(*otp))
+		return TEE_ERROR_BAD_PARAMETERS;
+
+	for (i = 0; i < otp_size; i++, u32++)


OP-TEE is very likely to always be little endian, especially where this PTA is compiled. How about adding a static_assert for that somewhere instead and use memcpy() directly?

Same for bytes_to_otp() below.

I wanted to be explicit about the data that is written into the OTP, because it's not really documented anywhere.

I dropped the functions and switched to memcpy() now.

How about adding a static_assert for that somewhere instead

I was looking for some existing helper or macro to check the endianess, but couldn't find anything. Is there anything that I missed and could just use?

Google it; there's a standard way for GCC, and I expect it to be compatible with clang.

jenswi-linaro · 2025-12-22T16:48:49Z

core/pta/rockchip/rk_secure_boot.c

+ * option is necessary to actually enable secure boot with PTA, but may
+ * potentially brick your device.
+ */
+static const int simulation = IS_ENABLED2(CFG_RK_SECURE_BOOT_SIMULATION);


CFG variables are better documented in the .mk file where they are introduced. Please use the IS_ENABLED() (yes, the version without the 2) macro directly instead.

jenswi-linaro · 2025-12-22T16:50:56Z

core/pta/rockchip/rk_secure_boot.c

+ */
+static const int simulation = IS_ENABLED2(CFG_RK_SECURE_BOOT_SIMULATION);
+
+static inline bool bit_test(uint32_t value, uint32_t bit)


The name of this function should be test_bits() or test_bit_mask() to better reflect what it's supposed to do. The function is too odd to be generically usable.

jenswi-linaro · 2025-12-22T16:54:39Z

core/pta/rockchip/rk_secure_boot.c

+	case 4096:
+		status |= ROCKCHIP_OTP_SECURE_BOOT_STATUS_RSA4096;
+
+		res = rockchip_otp_write_secure(&status,


Please make this (with the helper variables placed at the very beginning of the function):

uint32_t idx = ROCKCHIP_OTP_SECURE_BOOT_STATUS_INDEX; uint32_t sz = ROCKCHIP_OTP_SECURE_BOOT_STATUS_SIZE; ... res = rockchip_otp_write_secure(&status, idx, sz);

jenswi-linaro · 2025-12-22T16:55:58Z

core/pta/rockchip/rk_secure_boot.c

+		return NULL;
+
+	snprintf(str, str_size,
+		 "0x%" PRIx32 " 0x%" PRIx32 " 0x%" PRIx32 " 0x%" PRIx32


Please make this "0x%"PRIx32" 0x%"PRIx32" 0x%"PRIx32" 0x%"PRIx32, same for the line below.

jenswi-linaro · 2025-12-22T16:56:22Z

core/pta/rockchip/rk_secure_boot.c

+	TEE_Result res = TEE_SUCCESS;
+	uint32_t status = 0;
+
+	IMSG("Setting key size to %d", key_size_bits);


jenswi-linaro · 2025-12-22T16:57:38Z

core/pta/rockchip/rk_secure_boot.c

+	TEE_Result res = TEE_ERROR_GENERIC;
+	struct pta_rk_secure_boot_info *info = NULL;
+	uint32_t status = 0;
+	uint32_t hash[ROCKCHIP_OTP_RSA_HASH_SIZE];


Please initialize all locals and order them to create an upside-down tree when possible. Same for the rest of this pull request.

Done.

I don't like upside down tree very much, because it's solely stylistic, and I prefer to group variables that have similar use. But that's personal preference, and I will follow the suggested coding style.

jenswi-linaro · 2025-12-22T16:58:25Z

core/pta/rockchip/rk_secure_boot.c

+
+	key_size_bits = params[1].value.a;
+	if (key_size_bits != 4096 && key_size_bits != 2048) {
+		EMSG("Invalid key size: %d", key_size_bits);


jenswi-linaro · 2025-12-22T17:01:05Z

core/pta/rockchip/rk_secure_boot.c

+static char *otp_to_string(uint32_t *otp, size_t otp_size,
+			   char *str, size_t str_size)
+{
+	if (otp_size != 8 || str_size != HASH_STRING_SIZE)


These kind of tests only clutters the code. Use static_assert somewhere for the otp_size and let snprinf() itself deal with a too-short buffer.

I removed the check and the otp_size argument and added a static_assert for OTP_SIZE.

jenswi-linaro · 2025-12-22T17:07:47Z

core/pta/rockchip/rk_secure_boot.c

+		res = rockchip_otp_write_secure(&status,
+				ROCKCHIP_OTP_SECURE_BOOT_STATUS_INDEX,
+				ROCKCHIP_OTP_SECURE_BOOT_STATUS_SIZE);
+		if (res != TEE_SUCCESS)


Prefer if (res). Applies to the entire file.

jenswi-linaro · 2025-12-22T17:11:57Z

core/pta/rockchip/rk_secure_boot.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+
+	res = rockchip_otp_read_secure(old_hash,


Is there a risk of wearing out the otp if you read too many times too quickly?
It seems there are no restrictions on who may call this PTA.

Is there a risk of wearing out the otp if you read too many times too quickly?

I don't know. There is very little documentation available for the OTP.

There is a Linux driver for the non-secure OTP. That driver doesn't have any limitations on the read accesses.

On the other hand, it shouldn't be too difficult to add caching for the OTP values, since they usually won't change anyway.

Do you have additional information about these OTP that lead you to this question?

No, this was for a different platform 15+ years ago. Things might have changed. :-)

jenswi-linaro · 2025-12-29T16:36:50Z

Please squash the fixups into the main commit.

tretter · 2026-01-05T11:08:54Z

I squashed the fixup commits and rebased the branch on current master.

jenswi-linaro

With my comments addressed, please apply:
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

jenswi-linaro · 2026-01-05T13:08:07Z

core/pta/rockchip/rk_secure_boot.c

+				       ROCKCHIP_OTP_RSA_HASH_SIZE);
+	if (res)
+		return res;
+	if (memcmp(tmp, hash, sizeof(tmp)) != 0) {


Prefer if (memcmp(tmp, hash, sizeof(tmp)))

jenswi-linaro · 2026-01-05T13:09:22Z

core/pta/rockchip/rk_secure_boot.c

+				       ROCKCHIP_OTP_RSA_HASH_SIZE);
+	if (res)
+		return res;
+	if (memcmp(old_hash, new_hash, sizeof(new_hash)) != 0) {


Prefer if (memcmp(old_hash, new_hash, sizeof(new_hash)))

jenswi-linaro · 2026-01-05T13:09:50Z

core/pta/rockchip/rk_secure_boot.c

+				       ROCKCHIP_OTP_RSA_HASH_SIZE);
+	if (res)
+		return res;
+	if (memcmp(zero, hash, sizeof(hash)) == 0) {


if (!memcmp(zero, hash, sizeof(hash)))

The OTP area contains other values in addition to the HW_UNIQUE_KEY. For example, the SECURE_BOOT_STATUS and the RSA_HASH, which are used by the ROM code to verify booted images, are located there as well. Define the index (in 32 bit words) and length (in 32 bit words) of these values, too, to allow applications to read and write these locations. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

The S_OTP area for the Rockchip secure boot RSA hash and status register is accessible only from the secure world. Thus, secure boot must be enabled from the secure world on these board. The PTA implements 3 functions: 1. Ask the TA from the non-secure world about the current status and hash of the hardware. This allows to inspect the current status of secure boot on a specific device. 2. Write an RSA hash into the OTP fuses. It's the responsibility of the user to calculate the hash and ensure that it matches the key, which will be used to sign the images. 3. Actually lockdown the device by enabling secure boot. This is a separate step to allow the user to verify the setup before potentially bricking a device. With these functions, a user may use a client running in the normal world (for example in a boot loader or operating system) to enable secure boot on a Rockchip device. Implementing secure boot setup as an OP-TEE PTA has the advantage that secure boot can be enabled at any time during the device setup instead of during early boot. This allows a developer/user or additional scripts to interact with the secure boot setup process. The hash of the root key is accepted and reported as calculated by sha256sum and internally converted to the correct byte order that needs to be burned into the fuses. Signed-off-by: Michael Tretter <m.tretter@pengutronix.de> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

tretter · 2026-01-05T13:51:15Z

I addressed the comments regarding the return value of memcpy, already squashed the fixes into the respective commit, and added the Acked-by tag.

tretter · 2026-01-05T14:35:46Z

I just sent an RFC patch series for a user of the PTA implemented in barebox: https://lore.kernel.org/barebox/20260105-rockchip-secure-boot-v1-0-eaf5053a7d7e@pengutronix.de/T/#t

jenswi-linaro · 2026-01-05T17:09:43Z

I don't know what the problem is with checkpatch.

tretter mentioned this pull request Dec 17, 2025

Add a Rockchip secure boot PTA #7511

Closed

tretter force-pushed the rockchip-secure-boot branch from 5440ac0 to f7da2ad Compare December 17, 2025 16:36

tretter commented Dec 17, 2025

View reviewed changes

lib/libutee/include/pta_rk_secure_boot.h Outdated Show resolved Hide resolved

tretter commented Dec 17, 2025

View reviewed changes

lib/libutee/include/pta_rk_secure_boot.h Show resolved Hide resolved

tretter commented Dec 17, 2025

View reviewed changes

jenswi-linaro reviewed Dec 18, 2025

View reviewed changes

jenswi-linaro reviewed Dec 22, 2025

View reviewed changes

tretter force-pushed the rockchip-secure-boot branch from 07a6ab1 to 405e9b0 Compare January 5, 2026 10:32

jenswi-linaro reviewed Jan 5, 2026

View reviewed changes

tretter added 2 commits January 5, 2026 14:41

tretter force-pushed the rockchip-secure-boot branch from 405e9b0 to 6faa75b Compare January 5, 2026 13:43

jenswi-linaro merged commit 2949576 into OP-TEE:master Jan 5, 2026
111 of 113 checks passed

Add a Rockchip secure boot PTA #7661

Add a Rockchip secure boot PTA #7661

Conversation

tretter commented Dec 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment