Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <981c660f-27cb-5cbd-f965-3ecc1b36aa7a@linux.ibm.com>
Date:   Tue, 6 Jun 2023 16:18:43 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Content-Language: en-US
To:     Steffen Eiden <seiden@linux.ibm.com>, kvm@vger.kernel.org,
        linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        Viktor Mihajlovski <mihajlov@linux.ibm.com>
Cc:     Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Nico Boehr <nrb@linux.ibm.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Hendrik Brueckner <brueckner@linux.ibm.com>
References: <20230606113736.2934503-1-seiden@linux.ibm.com>
 <20230606113736.2934503-3-seiden@linux.ibm.com>
From:   Janosch Frank <frankja@linux.ibm.com>
Subject: Re: [PATCH v3 2/6] s390/uvdevice: Add 'Add Secret' UVC
In-Reply-To: <20230606113736.2934503-3-seiden@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/6/23 13:37, Steffen Eiden wrote:
> Userspace can call the Add Secret Ultravisor Call
> using IOCTLs on the uvdevice.
> During the handling of the new IOCTL nr the uvdevice will do some sanity
> checks first. Then, copy the request data to kernel space, perform the
> Ultravisor command, and copy the return codes to userspace.
> If the Add Secret UV facility is not present,
> UV will return invalid command rc. This won't be fenced in the driver
> and does not result in a negative return value. This is also true for
> any other possible error code the UV can return.

The Add Secret UV call sends an encrypted and cryptographically verified 
request to the Ultravisor. The request inserts a protected guest's 
secret into the Ultravisor for later use.

The uvdevice is merely transporting the request from userspace to the 
Ultravisor. It's neither checking nor manipulating the request data.

> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>   arch/s390/include/asm/uv.h            | 14 +++++++
>   arch/s390/include/uapi/asm/uvdevice.h |  4 ++
>   drivers/s390/char/uvdevice.c          | 57 +++++++++++++++++++++++++++
>   3 files changed, 75 insertions(+)
> 
> diff --git a/arch/s390/include/asm/uv.h b/arch/s390/include/asm/uv.h
> index 28a9ad57b6f1..1babc70ea5d4 100644
> --- a/arch/s390/include/asm/uv.h
> +++ b/arch/s390/include/asm/uv.h
> @@ -58,6 +58,7 @@
>   #define UVC_CMD_SET_SHARED_ACCESS	0x1000
>   #define UVC_CMD_REMOVE_SHARED_ACCESS	0x1001
>   #define UVC_CMD_RETR_ATTEST		0x1020
> +#define UVC_CMD_ADD_SECRET		0x1031
>   
>   /* Bits in installed uv calls */
>   enum uv_cmds_inst {
> @@ -88,6 +89,7 @@ enum uv_cmds_inst {
>   	BIT_UVC_CMD_DUMP_CPU = 26,
>   	BIT_UVC_CMD_DUMP_COMPLETE = 27,
>   	BIT_UVC_CMD_RETR_ATTEST = 28,
> +	BIT_UVC_CMD_ADD_SECRET = 29,
>   };
>   
>   enum uv_feat_ind {
> @@ -292,6 +294,18 @@ struct uv_cb_dump_complete {
>   	u64 reserved30[5];
>   } __packed __aligned(8);
>   
> +/*
> + * A common UV call struct for pv guests that contains a single address
> + * Examples:
> + * Add Secret
> + */
> +struct uv_cb_guest_addr {
> +	struct uv_cb_header header;
> +	u64 reserved08[3];
> +	u64 addr;
> +	u64 reserved28[4];
> +} __packed __aligned(8);
> +
>   static inline int __uv_call(unsigned long r1, unsigned long r2)
>   {
>   	int cc;
> diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
> index 9d9b684836c2..e77410226598 100644
> --- a/arch/s390/include/uapi/asm/uvdevice.h
> +++ b/arch/s390/include/uapi/asm/uvdevice.h
> @@ -69,6 +69,7 @@ struct uvio_uvdev_info {
>   #define UVIO_ATT_ARCB_MAX_LEN		0x100000
>   #define UVIO_ATT_MEASUREMENT_MAX_LEN	0x8000
>   #define UVIO_ATT_ADDITIONAL_MAX_LEN	0x8000
> +#define UVIO_ADD_SECRET_MAX_LEN		0x100000
>   
>   #define UVIO_DEVICE_NAME "uv"
>   #define UVIO_TYPE_UVC 'u'
> @@ -76,6 +77,7 @@ struct uvio_uvdev_info {
>   enum UVIO_IOCTL_NR {
>   	UVIO_IOCTL_UVDEV_INFO_NR = 0x00,
>   	UVIO_IOCTL_ATT_NR,
> +	UVIO_IOCTL_ADD_SECRET_NR,
>   	/* must be the last entry */
>   	UVIO_IOCTL_NUM_IOCTLS
>   };
> @@ -83,9 +85,11 @@ enum UVIO_IOCTL_NR {
>   #define UVIO_IOCTL(nr)		_IOWR(UVIO_TYPE_UVC, nr, struct uvio_ioctl_cb)
>   #define UVIO_IOCTL_UVDEV_INFO	UVIO_IOCTL(UVIO_IOCTL_UVDEV_INFO_NR)
>   #define UVIO_IOCTL_ATT		UVIO_IOCTL(UVIO_IOCTL_ATT_NR)
> +#define UVIO_IOCTL_ADD_SECRET	UVIO_IOCTL(UVIO_IOCTL_ADD_SECRET_NR)
>   
>   #define UVIO_SUPP_CALL(nr)	(1ULL << (nr))
>   #define UVIO_SUPP_UDEV_INFO	UVIO_SUPP_CALL(UVIO_IOCTL_UDEV_INFO_NR)
>   #define UVIO_SUPP_ATT		UVIO_SUPP_CALL(UVIO_IOCTL_ATT_NR)
> +#define UVIO_SUPP_ADD_SECRET	UVIO_SUPP_CALL(UVIO_IOCTL_ADD_SECRET_NR)
>   
>   #endif /* __S390_ASM_UVDEVICE_H */
> diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
> index 4efeebcaf382..c10554bc1fee 100644
> --- a/drivers/s390/char/uvdevice.c
> +++ b/drivers/s390/char/uvdevice.c
> @@ -37,6 +37,7 @@
>   static const u32 ioctl_nr_to_uvc_bit[] __initconst = {
>   	[UVIO_IOCTL_UVDEV_INFO_NR] = BIT_UVIO_INTERNAL,
>   	[UVIO_IOCTL_ATT_NR] = BIT_UVC_CMD_RETR_ATTEST,
> +	[UVIO_IOCTL_ADD_SECRET_NR] = BIT_UVC_CMD_ADD_SECRET,
>   };
>   
>   static_assert(ARRAY_SIZE(ioctl_nr_to_uvc_bit) == UVIO_IOCTL_NUM_IOCTLS);
> @@ -231,6 +232,59 @@ static int uvio_attestation(struct uvio_ioctl_cb *uv_ioctl)
>   	return ret;
>   }
>   
> +/** uvio_add_secret() - perform an Add Secret UVC
> + *
> + * @uv_ioctl: ioctl control block
> + *
> + * uvio_add_secret() performs the Add Secret Ultravisor Call. It verifies that
> + * the given userspace argument address is valid and its size is sane. Every
> + * other check is made by the Ultravisor (UV) and won't result in a negative
> + * return value. It copies the request to kernelspace, performs the UV-call, and
> + * copies the return codes to the ioctl control block. The argument has to point
> + * to an Add Secret Request Control Block. It is an encrypted and
> + * cryptographically verified request generated by userspace to insert the
> + * actual secret into the UV. If the Add Secret UV facility is not present, UV
> + * will return invalid command rc. This won't be fenced in the driver and does
> + * not result in a negative return value.
> + *
> + * Context: might sleep
> + *
> + * Return: 0 on success or a negative error code on error.
> + */

Maybe:

/** uvio_add_secret() - perform an Add Secret UVC 
 
 
  * 
 
 
  * @uv_ioctl: ioctl control block 
 
 
  * 
 
 
  * uvio_add_secret() performs the Add Secret Ultravisor Call. 
 
 
  * 
 
 
  * The given userspace argument address and size are verified to be 
 
 
  * valid but every other check is made by the Ultravisor 
 
 
  * (UV). Therefore UV errors won't result in a negative return 
 
 
  * value. The request is then copied to kernelspace, the UV-call is 
 
 
  * performed and the results are copied back to userspace. 
 
 
  * 
 
 
  * The argument has to point to an Add Secret Request Control Block 
 
 
  * which is an encrypted and cryptographically verified request that 
 
 
  * inserts a protected guest's secrets into the Ultravisor for later 
 
 
  * use. 
 
 
  * 
 
 
  * If the Add Secret UV facility is not present, UV will return 
 
 
  * invalid command rc. This won't be fenced in the driver and does not 
 
 
  * result in a negative return value. 
 
 
  * 
 
 
  * Context: might sleep 
 
 
  * 
 
 
  * Return: 0 on success or a negative error code on error. 
 
 
  */


> +static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
> +{
> +	void __user *user_buf_arg = (void __user *)uv_ioctl->argument_addr;
> +	struct uv_cb_guest_addr uvcb = {
> +		.header.len = sizeof(uvcb),
> +		.header.cmd = UVC_CMD_ADD_SECRET,
> +	};
> +	void *asrcb = NULL;
> +	int ret;
> +
> +	if (uv_ioctl->argument_len > UVIO_ADD_SECRET_MAX_LEN)
> +		return -EINVAL;
> +	if (uv_ioctl->argument_len == 0)
> +		return -EINVAL;
> +
> +	asrcb = kvzalloc(uv_ioctl->argument_len, GFP_KERNEL);
> +	if (!asrcb)
> +		return -ENOMEM;
> +
> +	ret = -EFAULT;
> +	if (copy_from_user(asrcb, user_buf_arg, uv_ioctl->argument_len))
> +		goto out;
> +
> +	ret = 0;
> +	uvcb.addr = (u64)asrcb;
> +	uv_call_sched(0, (u64)&uvcb);
> +	uv_ioctl->uv_rc = uvcb.header.rc;
> +	uv_ioctl->uv_rrc = uvcb.header.rrc;
> +
> +out:
> +	kvfree(asrcb);
> +	return ret;
> +}
> +
>   static int uvio_copy_and_check_ioctl(struct uvio_ioctl_cb *ioctl, void __user *argp,
>   				     unsigned long cmd)
>   {
> @@ -275,6 +329,9 @@ static long uvio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	case UVIO_IOCTL_ATT_NR:
>   		ret = uvio_attestation(&uv_ioctl);
>   		break;
> +	case UVIO_IOCTL_ADD_SECRET_NR:
> +		ret = uvio_add_secret(&uv_ioctl);
> +		break;
>   	default:
>   		ret = -ENOIOCTLCMD;
>   		break;