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

[Stefan Berger <stefanb@linux.ibm.com>]

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