[PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[Claudio Imbrenda]

The return value uv_set_shared() and uv_remove_shared() (which are
wrappers around the share() function) is not always checked. The system
integrity of a protected guest depends on the Share and Unshare UVCs
being successful. This means that any caller that fails to check the
return value will compromise the security of the protected guest.

No code path that would lead to such violation of the security
guarantees is currently exercised, since all the areas that are shared
never get unshared during the lifetime of the system. This might
change and become an issue in the future.

The Share and Unshare UVCs can only fail in case of hypervisor
misbehaviour (either a bug or malicious behaviour). In such cases there
is no reasonable way forward, and the system needs to panic.

This patch replaces the return at the end of the share() function with
a panic, to guarantee system integrity.

Fixes: 5abb9351dfd9 ("s390/uv: introduce guest side ultravisor code")
Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
---
 arch/s390/include/asm/uv.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
index 0b5f8f3e84f1..153d93468b77 100644
--- a/arch/s390/include/asm/uv.h
+++ b/arch/s390/include/asm/uv.h
@@ -441,7 +441,10 @@ static inline int share(unsigned long addr, u16 cmd)
 
 	if (!uv_call(0, (u64)&uvcb))
 		return 0;
-	return -EINVAL;
+	pr_err("%s UVC failed (rc: 0x%x, rrc: 0x%x), possible hypervisor bug.\n",
+	       uvcb.header.cmd == UVC_CMD_SET_SHARED_ACCESS ? "Share" : "Unshare",
+	       uvcb.header.rc, uvcb.header.rrc);
+	panic("System security cannot be guaranteed unless the system panics now.\n");
 }
 
 /*
-- 
2.45.2

Re: [PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[Janosch Frank]

On 8/1/24 1:25 PM, Claudio Imbrenda wrote:
> The return value uv_set_shared() and uv_remove_shared() (which are
> wrappers around the share() function) is not always checked. The system
> integrity of a protected guest depends on the Share and Unshare UVCs
> being successful. This means that any caller that fails to check the
> return value will compromise the security of the protected guest.
> 
> No code path that would lead to such violation of the security
> guarantees is currently exercised, since all the areas that are shared
> never get unshared during the lifetime of the system. This might
> change and become an issue in the future.

For people wondering what the effects might be, this is the important 
paragraph to read. Fortunately we're currently not unsharing anything.

Claudio already stated that there's no way out of this but I want to 
reiterate on this. The hypervisor has to mess up quite badly to force a 
rc > 0 for the guest. Likewise the guest has to mess up memory 
management to achieve a rc > 0.

The only time where the cause of the rc can be fixed is when the 
hypervisor is malicious and tracks its changes. In all other cases we 
won't know why we ended up with a rc and it makes sense to stop the VM 
before something worse happens.


@Claudio:
The patch subject is a bit non-specific.
How about:
"s390/uv: Panic for set and remove shared access UVC errors"

With that fixed:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[Claudio Imbrenda]

On Thu, 1 Aug 2024 15:20:30 +0200
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 8/1/24 1:25 PM, Claudio Imbrenda wrote:
> > The return value uv_set_shared() and uv_remove_shared() (which are
> > wrappers around the share() function) is not always checked. The system
> > integrity of a protected guest depends on the Share and Unshare UVCs
> > being successful. This means that any caller that fails to check the
> > return value will compromise the security of the protected guest.
> > 
> > No code path that would lead to such violation of the security
> > guarantees is currently exercised, since all the areas that are shared
> > never get unshared during the lifetime of the system. This might
> > change and become an issue in the future.  
> 
> For people wondering what the effects might be, this is the important 
> paragraph to read. Fortunately we're currently not unsharing anything.
> 
> Claudio already stated that there's no way out of this but I want to 
> reiterate on this. The hypervisor has to mess up quite badly to force a 
> rc > 0 for the guest. Likewise the guest has to mess up memory 
> management to achieve a rc > 0.
> 
> The only time where the cause of the rc can be fixed is when the 
> hypervisor is malicious and tracks its changes. In all other cases we 
> won't know why we ended up with a rc and it makes sense to stop the VM 
> before something worse happens.
> 
> 
> @Claudio:
> The patch subject is a bit non-specific.
> How about:
> "s390/uv: Panic for set and remove shared access UVC errors"

feel free to fix the subject when picking (unless you really want me to
send a v2)

> 
> With that fixed:
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
> 

Re: [PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[Steffen Eiden]

On 8/1/24 1:25 PM, Claudio Imbrenda wrote:
> The return value uv_set_shared() and uv_remove_shared() (which are
> wrappers around the share() function) is not always checked. The system
> integrity of a protected guest depends on the Share and Unshare UVCs
> being successful. This means that any caller that fails to check the
> return value will compromise the security of the protected guest.
> 
> No code path that would lead to such violation of the security
> guarantees is currently exercised, since all the areas that are shared
> never get unshared during the lifetime of the system. This might
> change and become an issue in the future.
> 
> The Share and Unshare UVCs can only fail in case of hypervisor
> misbehaviour (either a bug or malicious behaviour). In such cases there
> is no reasonable way forward, and the system needs to panic.
> 
> This patch replaces the return at the end of the share() function with
> a panic, to guarantee system integrity.
> 
> Fixes: 5abb9351dfd9 ("s390/uv: introduce guest side ultravisor code")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>
Reviewed-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   arch/s390/include/asm/uv.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 0b5f8f3e84f1..153d93468b77 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -441,7 +441,10 @@ static inline int share(unsigned long addr, u16 cmd)
>   
>   	if (!uv_call(0, (u64)&uvcb))
>   		return 0;
> -	return -EINVAL;
> +	pr_err("%s UVC failed (rc: 0x%x, rrc: 0x%x), possible hypervisor bug.\n",
> +	       uvcb.header.cmd == UVC_CMD_SET_SHARED_ACCESS ? "Share" : "Unshare",
> +	       uvcb.header.rc, uvcb.header.rrc);
> +	panic("System security cannot be guaranteed unless the system panics now.\n");
>   }
>   
>   /*

Re: [PATCH v1 1/1] s390/uv: Panic if the security of the system cannot be guaranteed.

[Christian Borntraeger]

Am 01.08.24 um 13:25 schrieb Claudio Imbrenda:
> The return value uv_set_shared() and uv_remove_shared() (which are
> wrappers around the share() function) is not always checked. The system
> integrity of a protected guest depends on the Share and Unshare UVCs
> being successful. This means that any caller that fails to check the
> return value will compromise the security of the protected guest.
> 
> No code path that would lead to such violation of the security
> guarantees is currently exercised, since all the areas that are shared
> never get unshared during the lifetime of the system. This might
> change and become an issue in the future.
> 
> The Share and Unshare UVCs can only fail in case of hypervisor
> misbehaviour (either a bug or malicious behaviour). In such cases there
> is no reasonable way forward, and the system needs to panic.
> 
> This patch replaces the return at the end of the share() function with
> a panic, to guarantee system integrity.
> 
> Fixes: 5abb9351dfd9 ("s390/uv: introduce guest side ultravisor code")
> Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   arch/s390/include/asm/uv.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 0b5f8f3e84f1..153d93468b77 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -441,7 +441,10 @@ static inline int share(unsigned long addr, u16 cmd)
>   
>   	if (!uv_call(0, (u64)&uvcb))
>   		return 0;
> -	return -EINVAL;
> +	pr_err("%s UVC failed (rc: 0x%x, rrc: 0x%x), possible hypervisor bug.\n",
> +	       uvcb.header.cmd == UVC_CMD_SET_SHARED_ACCESS ? "Share" : "Unshare",
> +	       uvcb.header.rc, uvcb.header.rrc);
> +	panic("System security cannot be guaranteed unless the system panics now.\n");
>   }
>   
>   /*