Re: [RFC PATCH 2/2] pseries: define sysfs interface to expose PKS variables

From: Dov Murik <dovmurik@linux.ibm.com>
To: Nayna Jain <nayna@linux.ibm.com>, linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org, Dov Murik <dovmurik@linux.ibm.com>,
	Douglas Miller <dougmill@linux.vnet.ibm.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	George Wilson <gcwilson@linux.ibm.com>,
	gjoyce@ibm.com, Daniel Axtens <dja@axtens.net>
Subject: Re: [RFC PATCH 2/2] pseries: define sysfs interface to expose PKS variables
Date: Wed, 9 Feb 2022 11:13:15 +0200	[thread overview]
Message-ID: <0d33cac6-99a7-e756-e0e3-37124784e3bb@linux.ibm.com> (raw)
In-Reply-To: <20220122005637.28199-3-nayna@linux.ibm.com>

Hi Nayna,

On 22/01/2022 2:56, Nayna Jain wrote:
> PowerVM guest secure boot intend to use Platform Keystore(PKS) for the
> purpose of storing public keys to verify digital signature.
> 
> Define sysfs interface to expose PKS variables to userspace to allow
> read/write/add/delete operations. Each variable is shown as a read/write
> attribute file. The size of the file represents the size of the current
> content of the variable.
> 
> create_var and delete_var attribute files are always present which allow
> users to create/delete variables. These are write only attributes.The
> design has tried to be compliant with sysfs semantic to represent single
> value per attribute. Thus, rather than mapping a complete data structure
> representation to create_var, it only accepts a single formatted string
> to create an empty variable.
> 
> The sysfs interface is designed such as to expose PKS configuration
> properties, operating system variables and firmware variables.
> Current version exposes configuration and operating system variables.
> The support for exposing firmware variables will be added in the future
> version.
> 
> Example of pksvar sysfs interface:
> 
> # cd /sys/firmware/pksvar/
> # ls
> config  os
> 
> # cd config
> 
> # ls -ltrh
> total 0
> -r--r--r-- 1 root root 64K Jan 21 17:55 version
> -r--r--r-- 1 root root 64K Jan 21 17:55 used_space
> -r--r--r-- 1 root root 64K Jan 21 17:55 total_size
> -r--r--r-- 1 root root 64K Jan 21 17:55 supported_policies
> -r--r--r-- 1 root root 64K Jan 21 17:55 max_object_size
> -r--r--r-- 1 root root 64K Jan 21 17:55 max_object_label_size
> -r--r--r-- 1 root root 64K Jan 21 17:55 flags
> 
> # cd os
> 
> # ls -ltrh
> total 0
> -rw------- 1 root root 104 Jan 21 17:56 var4
> -rw------- 1 root root 104 Jan 21 17:56 var3
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_PK
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_KEK
> -rw------- 1 root root  76 Jan 21 17:56 GLOBAL_dbx
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_db
> --w------- 1 root root 64K Jan 21 17:56 delete_var
> --w------- 1 root root 64K Jan 21 17:56 create_var
> 
> 1. Read variable
> 
> # hexdump -C GLOBAL_db
> 00000000  00 00 00 00 a1 59 c0 a5  e4 94 a7 4a 87 b5 ab 15  |.....Y.....J....|
> 00000010  5c 2b f0 72 3f 03 00 00  00 00 00 00 23 03 00 00  |\+.r?.......#...|
> ....
> 00000330  02 a8 e8 ed 0f 20 60 3f  40 04 7c a8 91 21 37 eb  |..... `?@.|..!7.|
> 00000340  f3 f1 4e                                          |..N|
> 00000343
> 
> 2. Write variable
> 
> cat /tmp/data.bin > <variable_name>
> 
> 3. Create variable
> 
> # echo "var1" > create_var

It would be easier to understand if the user could create a new variable
like a regular new file, something like:

# cat /tmp/data.bin > var1

but I understand there are also comma-seperated-policy-strings which
should go somewhere; not sure how this fits (or if there are other
examples for similar interfaces in other sysfs parts).

> # ls -ltrh
> total 0
> -rw------- 1 root root 104 Jan 21 17:56 var4
> -rw------- 1 root root 104 Jan 21 17:56 var3
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_PK
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_KEK
> -rw------- 1 root root  76 Jan 21 17:56 GLOBAL_dbx
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_db
> --w------- 1 root root 64K Jan 21 17:56 delete_var
> --w------- 1 root root 64K Jan 21 17:57 create_var
> -rw------- 1 root root   0 Jan 21 17:57 var1.tmp
> 
> Current design creates a zero size temporary variable. This implies
> it is not yet persisted to PKS. Only once data is written to newly
> created temporary variable and if it is successfully stored in the
> PKS, that the variable is permanent. The temporary variable will get
> removed on reboot. The code currently doesn't remove .tmp suffix
> immediately when persisted. The future version will fix this.
> 
> To avoid the additional .tmp semantic, alternative option is to consider
> any zero size variable as temporary variable. This option is under
> evaluation. This would avoid any runtime sysfs magic to replace .tmp
> variable with real variable.
> 
> Also, the formatted string to pass to create_var will have following
> format in the future version:
> <variable_name>:<comma-separated-policy strings>
> 
> 4. Delete variable
> # echo "var3" > delete_var

If it's possible here, I think it would be easier to understand (and
use) if you implement unlink(), so deleting var3 would be:

# rm var3

(and then there's no need for the special 'delete_var' entry.)

-Dov

> # ls -ltrh
> total 0
> -rw------- 1 root root 104 Jan 21 17:56 var4
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_PK
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_KEK
> -rw------- 1 root root  76 Jan 21 17:56 GLOBAL_dbx
> -rw------- 1 root root 831 Jan 21 17:56 GLOBAL_db
> --w------- 1 root root 64K Jan 21 17:57 create_var
> -rw------- 1 root root   0 Jan 21 17:57 var1.tmp
> --w------- 1 root root 64K Jan 21 17:58 delete_var
> 
> The var3 file is removed at runtime, if variable is successfully
> removed from the PKS storage.
> 
> NOTE: We are evaluating two design for userspace interface: using the
> sysfs or defining a new filesystem based. Any feedback on this sysfs based
> approach would be highly appreciated. We have tried to follow one value
> per attribute semantic. If that or any other semantics aren't followed
> properly, please let us know.
> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-pksvar        |  77 ++++
>  arch/powerpc/platforms/pseries/Kconfig        |   7 +
>  arch/powerpc/platforms/pseries/Makefile       |   1 +
>  arch/powerpc/platforms/pseries/pksvar-sysfs.c | 356 ++++++++++++++++++
>  4 files changed, 441 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-pksvar
>  create mode 100644 arch/powerpc/platforms/pseries/pksvar-sysfs.c
> 