* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 14:30 [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails Thomas Huth
@ 2024-01-09 14:42 ` Daniel P. Berrangé
2024-01-09 14:52 ` Thomas Huth
2024-01-09 15:34 ` Claudio Imbrenda
2024-01-09 16:51 ` Cédric Le Goater
2 siblings, 1 reply; 7+ messages in thread
From: Daniel P. Berrangé @ 2024-01-09 14:42 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-s390x, Christian Borntraeger, David Hildenbrand,
Claudio Imbrenda, Janosch Frank, qemu-devel, Halil Pasic
On Tue, Jan 09, 2024 at 03:30:38PM +0100, Thomas Huth wrote:
> It's a common scenario to copy guest images from one host to another
> to run the guest on the other machine. This (of course) does not work
> with "secure exection" guests since they are encrypted with one certain
> host key. However, if you still (accidentally) do it, you only get a
> very user-unfriendly error message that looks like this:
Not a comment on the patch, but my own interest how/where does the
disk image encryption/decryption happen ? Is that in guest kernel
context, and any info on what format the encryption uses ?
>
> qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
> header rc 108 rrc 5 IOCTL rc: -22
>
> Let's provide at least a somewhat nicer hint to the users so that they
> are able to figure out what might have gone wrong.
>
> Buglink: https://issues.redhat.com/browse/RHEL-18212
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> target/s390x/kvm/pv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index 6a69be7e5c..2833a255fa 100644
> --- a/target/s390x/kvm/pv.c
> +++ b/target/s390x/kvm/pv.c
> @@ -29,7 +29,8 @@ static bool info_valid;
> static struct kvm_s390_pv_info_vm info_vm;
> static struct kvm_s390_pv_info_dump info_dump;
>
> -static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> +static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
> + int *pvrc)
> {
> struct kvm_pv_cmd pv_cmd = {
> .cmd = cmd,
> @@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
> rc);
> }
> + if (pvrc) {
> + *pvrc = pv_cmd.rc;
> + }
> return rc;
> }
>
> @@ -53,12 +57,13 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> * This macro lets us pass the command as a string to the function so
> * we can print it on an error.
> */
> -#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data)
> +#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
> +#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, pvrc)
> #define s390_pv_cmd_exit(cmd, data) \
> { \
> int rc; \
> \
> - rc = __s390_pv_cmd(cmd, #cmd, data);\
> + rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
> if (rc) { \
> exit(1); \
> } \
> @@ -144,12 +149,19 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>
> int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> {
> + int ret, pvrc;
> struct kvm_s390_pv_sec_parm args = {
> .origin = origin,
> .length = length,
> };
>
> - return s390_pv_cmd(KVM_PV_SET_SEC_PARMS, &args);
> + ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
> + if (ret && pvrc == 0x108) {
> + error_report("Can't set secure parameters, please check whether "
> + "the image is correctly encrypted for this host");
> + }
> +
> + return ret;
> }
>
> /*
> --
> 2.43.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 14:42 ` Daniel P. Berrangé
@ 2024-01-09 14:52 ` Thomas Huth
2024-01-09 15:36 ` Janosch Frank
0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2024-01-09 14:52 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: qemu-s390x, Christian Borntraeger, David Hildenbrand,
Claudio Imbrenda, Janosch Frank, qemu-devel, Halil Pasic
On 09/01/2024 15.42, Daniel P. Berrangé wrote:
> On Tue, Jan 09, 2024 at 03:30:38PM +0100, Thomas Huth wrote:
>> It's a common scenario to copy guest images from one host to another
>> to run the guest on the other machine. This (of course) does not work
>> with "secure exection" guests since they are encrypted with one certain
>> host key. However, if you still (accidentally) do it, you only get a
>> very user-unfriendly error message that looks like this:
>
> Not a comment on the patch, but my own interest how/where does the
> disk image encryption/decryption happen ? Is that in guest kernel
> context, and any info on what format the encryption uses ?
There is an "ultravisor" (part of the host firmware) that takes care of the
decryption. See e.g. Claudio's talk here:
https://www.youtube.com/watch?v=J2YibrLfB4s
HTH,
Thomas
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 14:52 ` Thomas Huth
@ 2024-01-09 15:36 ` Janosch Frank
0 siblings, 0 replies; 7+ messages in thread
From: Janosch Frank @ 2024-01-09 15:36 UTC (permalink / raw)
To: Thomas Huth, Daniel P. Berrangé
Cc: qemu-s390x, Christian Borntraeger, David Hildenbrand,
Claudio Imbrenda, qemu-devel, Halil Pasic, Marc Hartmayer
On 1/9/24 15:52, Thomas Huth wrote:
> On 09/01/2024 15.42, Daniel P. Berrangé wrote:
>> On Tue, Jan 09, 2024 at 03:30:38PM +0100, Thomas Huth wrote:
>>> It's a common scenario to copy guest images from one host to another
>>> to run the guest on the other machine. This (of course) does not work
>>> with "secure exection" guests since they are encrypted with one certain
>>> host key. However, if you still (accidentally) do it, you only get a
>>> very user-unfriendly error message that looks like this:
>>
>> Not a comment on the patch, but my own interest how/where does the
>> disk image encryption/decryption happen ? Is that in guest kernel
>> context, and any info on what format the encryption uses ?
>
> There is an "ultravisor" (part of the host firmware) that takes care of the
> decryption. See e.g. Claudio's talk here:
>
> https://www.youtube.com/watch?v=J2YibrLfB4s
And here's the tool that creates the encrypted image:
https://github.com/ibm-s390-linux/s390-tools/tree/master/genprotimg
If I remember correctly the image should be aes-256-xts.
The SE header (that contains the image key) should be aes-256-gcm.
The header has keyslots so each machine the VM is allowed to run on can
unwrap the header independently.
Adding Marc to keep me honest here since he wrote genprotimg.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 14:30 [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails Thomas Huth
2024-01-09 14:42 ` Daniel P. Berrangé
@ 2024-01-09 15:34 ` Claudio Imbrenda
2024-01-09 16:51 ` Cédric Le Goater
2 siblings, 0 replies; 7+ messages in thread
From: Claudio Imbrenda @ 2024-01-09 15:34 UTC (permalink / raw)
To: Thomas Huth
Cc: qemu-s390x, Christian Borntraeger, David Hildenbrand,
Janosch Frank, qemu-devel, Halil Pasic
On Tue, 9 Jan 2024 15:30:38 +0100
Thomas Huth <thuth@redhat.com> wrote:
> It's a common scenario to copy guest images from one host to another
> to run the guest on the other machine. This (of course) does not work
> with "secure exection" guests since they are encrypted with one certain
"secure execution"
> host key. However, if you still (accidentally) do it, you only get a
> very user-unfriendly error message that looks like this:
>
> qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
> header rc 108 rrc 5 IOCTL rc: -22
>
> Let's provide at least a somewhat nicer hint to the users so that they
> are able to figure out what might have gone wrong.
>
> Buglink: https://issues.redhat.com/browse/RHEL-18212
> Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
> ---
> target/s390x/kvm/pv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index 6a69be7e5c..2833a255fa 100644
> --- a/target/s390x/kvm/pv.c
> +++ b/target/s390x/kvm/pv.c
> @@ -29,7 +29,8 @@ static bool info_valid;
> static struct kvm_s390_pv_info_vm info_vm;
> static struct kvm_s390_pv_info_dump info_dump;
>
> -static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> +static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
> + int *pvrc)
> {
> struct kvm_pv_cmd pv_cmd = {
> .cmd = cmd,
> @@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
> rc);
> }
> + if (pvrc) {
> + *pvrc = pv_cmd.rc;
> + }
> return rc;
> }
>
> @@ -53,12 +57,13 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> * This macro lets us pass the command as a string to the function so
> * we can print it on an error.
> */
> -#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data)
> +#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
> +#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, pvrc)
> #define s390_pv_cmd_exit(cmd, data) \
> { \
> int rc; \
> \
> - rc = __s390_pv_cmd(cmd, #cmd, data);\
> + rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
> if (rc) { \
> exit(1); \
> } \
> @@ -144,12 +149,19 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>
> int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> {
> + int ret, pvrc;
> struct kvm_s390_pv_sec_parm args = {
> .origin = origin,
> .length = length,
> };
>
> - return s390_pv_cmd(KVM_PV_SET_SEC_PARMS, &args);
> + ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
> + if (ret && pvrc == 0x108) {
> + error_report("Can't set secure parameters, please check whether "
> + "the image is correctly encrypted for this host");
> + }
> +
> + return ret;
> }
>
> /*
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 14:30 [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails Thomas Huth
2024-01-09 14:42 ` Daniel P. Berrangé
2024-01-09 15:34 ` Claudio Imbrenda
@ 2024-01-09 16:51 ` Cédric Le Goater
2024-01-10 12:09 ` Thomas Huth
2 siblings, 1 reply; 7+ messages in thread
From: Cédric Le Goater @ 2024-01-09 16:51 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, Christian Borntraeger, David Hildenbrand,
Claudio Imbrenda, Janosch Frank
Cc: qemu-devel, Halil Pasic
On 1/9/24 15:30, Thomas Huth wrote:
> It's a common scenario to copy guest images from one host to another
> to run the guest on the other machine. This (of course) does not work
> with "secure exection" guests since they are encrypted with one certain
> host key. However, if you still (accidentally) do it, you only get a
> very user-unfriendly error message that looks like this:
>
> qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
> header rc 108 rrc 5 IOCTL rc: -22
>
> Let's provide at least a somewhat nicer hint to the users so that they
> are able to figure out what might have gone wrong.
>
> Buglink: https://issues.redhat.com/browse/RHEL-18212
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> target/s390x/kvm/pv.c | 20 ++++++++++++++++----
> 1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
> index 6a69be7e5c..2833a255fa 100644
> --- a/target/s390x/kvm/pv.c
> +++ b/target/s390x/kvm/pv.c
> @@ -29,7 +29,8 @@ static bool info_valid;
> static struct kvm_s390_pv_info_vm info_vm;
> static struct kvm_s390_pv_info_dump info_dump;
>
> -static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> +static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
> + int *pvrc)
> {
> struct kvm_pv_cmd pv_cmd = {
> .cmd = cmd,
> @@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
> rc);
> }
> + if (pvrc) {
> + *pvrc = pv_cmd.rc;
> + }
> return rc;
> }
>
> @@ -53,12 +57,13 @@ static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
> * This macro lets us pass the command as a string to the function so
> * we can print it on an error.
> */
> -#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data)
> +#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
> +#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data, pvrc)
> #define s390_pv_cmd_exit(cmd, data) \
> { \
> int rc; \
> \
> - rc = __s390_pv_cmd(cmd, #cmd, data);\
> + rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
> if (rc) { \
> exit(1); \
> } \
> @@ -144,12 +149,19 @@ bool s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>
> int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> {
> + int ret, pvrc;
> struct kvm_s390_pv_sec_parm args = {
> .origin = origin,
> .length = length,
> };
>
> - return s390_pv_cmd(KVM_PV_SET_SEC_PARMS, &args);
> + ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
> + if (ret && pvrc == 0x108) {
why do we need to test for 0x108 also ? if this sub error code is important,
adding a define would be a plus.
> + error_report("Can't set secure parameters, please check whether "
> + "the image is correctly encrypted for this host");
The error reporting in s390x_machine_protect() could be improved.
I would add a 'Error *' argument to the routines called by
s390x_machine_protect() and report the error in s390x_machine_protect()
or above. s390_machine_protect() return value is ignored also, could be
replaced by a bool.
Thanks,
C.
> + }
> +
> + return ret;
> }
>
> /*
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails
2024-01-09 16:51 ` Cédric Le Goater
@ 2024-01-10 12:09 ` Thomas Huth
0 siblings, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2024-01-10 12:09 UTC (permalink / raw)
To: Cédric Le Goater, qemu-s390x, Christian Borntraeger,
David Hildenbrand, Claudio Imbrenda, Janosch Frank
Cc: qemu-devel, Halil Pasic
On 09/01/2024 17.51, Cédric Le Goater wrote:
>
>
> On 1/9/24 15:30, Thomas Huth wrote:
>> It's a common scenario to copy guest images from one host to another
>> to run the guest on the other machine. This (of course) does not work
>> with "secure exection" guests since they are encrypted with one certain
>> host key. However, if you still (accidentally) do it, you only get a
>> very user-unfriendly error message that looks like this:
>>
>> qemu-system-s390x: KVM PV command 2 (KVM_PV_SET_SEC_PARMS) failed:
>> header rc 108 rrc 5 IOCTL rc: -22
>>
>> Let's provide at least a somewhat nicer hint to the users so that they
>> are able to figure out what might have gone wrong.
>>
>> Buglink: https://issues.redhat.com/browse/RHEL-18212
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> target/s390x/kvm/pv.c | 20 ++++++++++++++++----
>> 1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/s390x/kvm/pv.c b/target/s390x/kvm/pv.c
>> index 6a69be7e5c..2833a255fa 100644
>> --- a/target/s390x/kvm/pv.c
>> +++ b/target/s390x/kvm/pv.c
>> @@ -29,7 +29,8 @@ static bool info_valid;
>> static struct kvm_s390_pv_info_vm info_vm;
>> static struct kvm_s390_pv_info_dump info_dump;
>> -static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data)
>> +static int __s390_pv_cmd(uint32_t cmd, const char *cmdname, void *data,
>> + int *pvrc)
>> {
>> struct kvm_pv_cmd pv_cmd = {
>> .cmd = cmd,
>> @@ -46,6 +47,9 @@ static int __s390_pv_cmd(uint32_t cmd, const char
>> *cmdname, void *data)
>> "IOCTL rc: %d", cmd, cmdname, pv_cmd.rc, pv_cmd.rrc,
>> rc);
>> }
>> + if (pvrc) {
>> + *pvrc = pv_cmd.rc;
>> + }
>> return rc;
>> }
>> @@ -53,12 +57,13 @@ static int __s390_pv_cmd(uint32_t cmd, const char
>> *cmdname, void *data)
>> * This macro lets us pass the command as a string to the function so
>> * we can print it on an error.
>> */
>> -#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data)
>> +#define s390_pv_cmd(cmd, data) __s390_pv_cmd(cmd, #cmd, data, NULL)
>> +#define s390_pv_cmd_pvrc(cmd, data, pvrc) __s390_pv_cmd(cmd, #cmd, data,
>> pvrc)
>> #define s390_pv_cmd_exit(cmd, data) \
>> { \
>> int rc; \
>> \
>> - rc = __s390_pv_cmd(cmd, #cmd, data);\
>> + rc = __s390_pv_cmd(cmd, #cmd, data, NULL); \
>> if (rc) { \
>> exit(1); \
>> } \
>> @@ -144,12 +149,19 @@ bool
>> s390_pv_vm_try_disable_async(S390CcwMachineState *ms)
>> int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>> {
>> + int ret, pvrc;
>> struct kvm_s390_pv_sec_parm args = {
>> .origin = origin,
>> .length = length,
>> };
>> - return s390_pv_cmd(KVM_PV_SET_SEC_PARMS, &args);
>> + ret = s390_pv_cmd_pvrc(KVM_PV_SET_SEC_PARMS, &args, &pvrc);
>> + if (ret && pvrc == 0x108) {
>
> why do we need to test for 0x108 also ? if this sub error code is important,
> adding a define would be a plus.
As far as I understood, other codes here could indicate a different failure,
so I wanted to make sure to limit the text to this scenario only.
And AFAIK the error codes are something internal to the ultravisor, not
documented publicly, so coming up with a #define here sounds hard to me.
>> + error_report("Can't set secure parameters, please check whether "
>> + "the image is correctly encrypted for this host");
> The error reporting in s390x_machine_protect() could be improved.
>
> I would add a 'Error *' argument to the routines called by
> s390x_machine_protect() and report the error in s390x_machine_protect()
> or above. s390_machine_protect() return value is ignored also, could be
> replaced by a bool.
Ok, I'll give it a try and send a v2.
Thomas
^ permalink raw reply [flat|nested] 7+ messages in thread