Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic secure boot

On 2/13/23 06:32, Michael Ellerman wrote:

Stefan Berger [off-list ref] writes:

quoted

On 2/10/23 03:03, Andrew Donnellan wrote:

quoted

From: Russell Currey <redacted>

...

quoted

quoted

+static int plpks_set_variable(const char *key, u64 key_len, u8 *data,
+			      u64 data_size)
+{
+	struct plpks_var var = {0};
+	int rc = 0;
+	u64 flags;
+
+	// Secure variables need to be prefixed with 8 bytes of flags.
+	// We only want to perform the write if we have at least one byte of data.
+	if (data_size <= sizeof(flags))
+		return -EINVAL;
+
+	// We subtract 1 from key_len because we don't need to include the
+	// null terminator at the end of the string
+	var.name = kcalloc(key_len - 1, sizeof(wchar_t), GFP_KERNEL);
+	if (!var.name)
+		return -ENOMEM;
+	rc = utf8s_to_utf16s(key, key_len - 1, UTF16_LITTLE_ENDIAN, (wchar_t *)var.name,
+			     key_len - 1);
+	if (rc < 0)
+		goto err;
+	var.namelen = rc * 2;
+
+	memcpy(&flags, data, sizeof(flags));

conversion from bytestream to integer: I think in this case it would be better to use

flags = cpu_to_be64p((__u64*)data);

so that the flags always in hypervisor/big endian format

I don't think it's correct to byte swap the flags here. They must be in
big endian format, but that's up to the caller.

The powernv secvar backend doesn't byte swap the flags, if the pseries
one did then the final content of the variable, written either by phyp
or OPAL, would differ depending on which backend is active.

Or am I missing something?

It seems wrong to not use the cpu_to_be64p() API to convert a byte stream to flags... That's why I suggested this.

    Stefan

cheers