[PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Thomas Huth]

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) {
+        error_report("Can't set secure parameters, please check whether "
+                     "the image is correctly encrypted for this host");
+    }
+
+    return ret;
 }
 
 /*
-- 
2.43.0

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Daniel P. Berrangé]

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 :|

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Thomas Huth]

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

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Claudio Imbrenda]

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;
>  }
>  
>  /*

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Janosch Frank]

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.

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Cédric Le Goater]

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;
>   }
>   
>   /*

Re: [PATCH] target/s390x/kvm/pv: Provide some more useful information if decryption fails

[Thomas Huth]

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