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

Stefan Berger
2023-02-13T13:52:54Z

On 2/13/23 06:32, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> writes:
>> On 2/10/23 03:03, Andrew Donnellan wrote:
>>> From: Russell Currey <ruscur@russell.cc>
> ...
>>> +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