Re: [kvm-unit-tests PATCH v2 2/2] s390x: create persistent comm-key

[Janosch Frank <frankja@linux.ibm.com>]

On 8/25/22 15:16, Nico Boehr wrote:
> To decrypt the dump of a PV guest, the comm-key (CCK) is required. Until
> now, no comm-key was provided to genprotimg, therefore decrypting the
> dump of a kvm-unit-test under PV was not possible.
> 
> This patch makes sure that we create a random CCK if there's no
> $(TEST_DIR)/comm.key file.
> 
> Also allow dumping of PV tests by passing the appropriate PCF to
> genprotimg (bit 34). --x-pcf is used to be compatible with older
> genprotimg versions, which don't support --enable-dump. 0xe0 is the
> default PCF value and only bit 34 is added.
> 
> Unfortunately, recent versions of genprotimg removed the --x-comm-key
> argument which was used by older versions to specify the CCK. To support
> these versions, we need to parse the genprotimg help output and decide
> which argument to use.
> 
> Signed-off-by: Nico Boehr <nrb@linux.ibm.com>

There are three minor issues that keep me from picking this

> ---
>   s390x/Makefile | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/s390x/Makefile b/s390x/Makefile
> index d17055ebe6a8..4e268f47b6ab 100644
> --- a/s390x/Makefile
> +++ b/s390x/Makefile
> @@ -162,15 +162,30 @@ $(SNIPPET_DIR)/c/%.hdr: $(SNIPPET_DIR)/c/%.gbin $(HOST_KEY_DOCUMENT)
>   	$(RM) $(@:.elf=.aux.o)
>   	@chmod a-x $@
>   
> +# Secure Execution Customer Communication Key file
> +# 32 bytes of key material, uses existing one if available
> +comm-key = $(TEST_DIR)/comm.key
> +$(comm-key):
> +	dd if=/dev/urandom of=$@ bs=32 count=1 status=none
> +
>   %.bin: %.elf
>   	$(OBJCOPY) -O binary  $< $@
>   
> -genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify

Add comment:


# The genprotimg arguments for the cck changed over time so we need to 
figure out which argument to use in order to set the cck

> +GENPROTIMG_HAS_COMM_KEY = $(shell $(GENPROTIMG) --help | grep -q -- --comm-key && echo yes)
> +ifeq ($(GENPROTIMG_HAS_COMM_KEY),yes)
> +	GENPROTIMG_COMM_KEY = --comm-key $(comm-key)
> +else
> +	GENPROTIMG_COMM_KEY = --x-comm-key $(comm-key)
> +endif

I'd like to have a \n here as well

> +# use x-pcf to be compatible with old genprotimg versions
> +# allow dumping + PCKMO
> +genprotimg_pcf = 0x200000e0
> +genprotimg_args = --host-key-document $(HOST_KEY_DOCUMENT) --no-verify $(GENPROTIMG_COMM_KEY) --x-pcf $(genprotimg_pcf)
>   
> -%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@)
> +%selftest.pv.bin: %selftest.bin $(HOST_KEY_DOCUMENT) $(patsubst %.pv.bin,%.parmfile,$@) $(comm-key)
>   	$(GENPROTIMG) $(genprotimg_args) --parmfile $(patsubst %.pv.bin,%.parmfile,$@) --image $< -o $@
>   
> -%.pv.bin: %.bin $(HOST_KEY_DOCUMENT)
> +%.pv.bin: %.bin $(HOST_KEY_DOCUMENT) $(comm-key)
>   	$(GENPROTIMG) $(genprotimg_args) --image $< -o $@
>   
>   $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
> @@ -178,7 +193,7 @@ $(snippet_asmlib): $$(patsubst %.o,%.S,$$@) $(asm-offsets)
>   
>   
>   arch_clean: asm_offsets_clean
> -	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d
> +	$(RM) $(TEST_DIR)/*.{o,elf,bin} $(SNIPPET_DIR)/*/*.{o,elf,*bin,*obj,hdr} $(SNIPPET_DIR)/asm/.*.d $(TEST_DIR)/.*.d lib/s390x/.*.d $(comm-key)

Hmmmmmmm(TM)

My first thought was that I'd be pretty angry if the CCK changes on a 
distclean. But the only scenario where this would matter is when the 
tests are provided to another system.

I'm still a bit torn about deleting the CCK especially as there will 
always be a CCK in the SE header no matter if we specify one or not.

>   
>   generated-files = $(asm-offsets)
>   $(tests:.elf=.o) $(asmlib) $(cflatobjs): $(generated-files)