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

[Stefan Berger]

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