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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Feb 2023 08:52:26 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Michael Ellerman <michaele@....ibm.com>,
        Andrew Donnellan <ajd@...ux.ibm.com>,
        linuxppc-dev@...ts.ozlabs.org, linux-integrity@...r.kernel.org
Cc:     ruscur@...sell.cc, bgray@...ux.ibm.com, nayna@...ux.ibm.com,
        gcwilson@...ux.ibm.com, gjoyce@...ux.ibm.com, brking@...ux.ibm.com,
        sudhakar@...ux.ibm.com, erichte@...ux.ibm.com,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        zohar@...ux.ibm.com, joel@....id.au, npiggin@...il.com
Subject: Re: [PATCH v6 24/26] powerpc/pseries: Implement secvars for dynamic
 secure boot



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