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]

Message-ID: <87pmadvm0n.fsf@mpe.ellerman.id.au>
Date:   Mon, 13 Feb 2023 22:32:24 +1100
From:   Michael Ellerman <michaele@....ibm.com>
To:     Stefan Berger <stefanb@...ux.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

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?

cheers