Message-ID: <f852f3d3-3d17-5204-1590-8add97033a6f@linux.ibm.com>
Date:   Mon, 13 Feb 2023 08:52:26 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic
 secure boot
Content-Language: en-US
To:     Michael Ellerman <michaele@au1.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
References: <20230210080401.345462-1-ajd@linux.ibm.com>
 <20230210080401.345462-25-ajd@linux.ibm.com>
 <f35e9ba1-5fdb-4cfa-5b41-cc55307dcd45@linux.ibm.com>
 <87pmadvm0n.fsf@mpe.ellerman.id.au>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <87pmadvm0n.fsf@mpe.ellerman.id.au>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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