From:   Michael Ellerman <michaele@au1.ibm.com>
To:     Stefan Berger <stefanb@linux.ibm.com>,
        Andrew Donnellan <ajd@linux.ibm.com>,
        linuxppc-dev@lists.ozlabs.org, linux-integrity@vger.kernel.org
Cc:     ruscur@russell.cc, bgray@linux.ibm.com, nayna@linux.ibm.com,
        gcwilson@linux.ibm.com, gjoyce@linux.ibm.com, brking@linux.ibm.com,
        sudhakar@linux.ibm.com, erichte@linux.ibm.com,
        gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
        zohar@linux.ibm.com, joel@jms.id.au, npiggin@gmail.com
Subject: Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic
 secure boot
In-Reply-To: <f35e9ba1-5fdb-4cfa-5b41-cc55307dcd45@linux.ibm.com>
References: <20230210080401.345462-1-ajd@linux.ibm.com>
 <20230210080401.345462-25-ajd@linux.ibm.com>
 <f35e9ba1-5fdb-4cfa-5b41-cc55307dcd45@linux.ibm.com>
Date:   Mon, 13 Feb 2023 22:32:24 +1100
Message-ID: <87pmadvm0n.fsf@mpe.ellerman.id.au>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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?

cheers