[PATCH v2] s390/uvdevice: Support longer secret lists

Steffen Eiden posted 1 patch 3 weeks, 3 days ago
There is a newer version of this series
arch/s390/include/uapi/asm/uvdevice.h |  1 +
drivers/s390/char/uvdevice.c          | 72 ++++++++++++++++++++-------
2 files changed, 54 insertions(+), 19 deletions(-)
[PATCH v2] s390/uvdevice: Support longer secret lists
Posted by Steffen Eiden 3 weeks, 3 days ago
Enable the list IOCTL to provide lists longer than on page (85 entries).
The list IOCTL accepts argument length up to 8 pages in page granularity
and will fill the argument up to this length with entries until the list
ends. User space unaware of this enhancement will still receive one page
of data and an uv_rc 0x0100.

Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
---
Reworked the whole list-creation loop. Hardened+simplified the implementation.
Now, only the actual data filled by the CP is copied to userspace.

 arch/s390/include/uapi/asm/uvdevice.h |  1 +
 drivers/s390/char/uvdevice.c          | 72 ++++++++++++++++++++-------
 2 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
index 4947f26ad9fb..c584250d4a35 100644
--- a/arch/s390/include/uapi/asm/uvdevice.h
+++ b/arch/s390/include/uapi/asm/uvdevice.h
@@ -71,6 +71,7 @@ struct uvio_uvdev_info {
 #define UVIO_ATT_ADDITIONAL_MAX_LEN	0x8000
 #define UVIO_ADD_SECRET_MAX_LEN		0x100000
 #define UVIO_LIST_SECRETS_LEN		0x1000
+#define UVIO_LIST_SECRETS_MAX_LEN	0x8000
 #define UVIO_RETR_SECRET_MAX_LEN	0x2000
 
 #define UVIO_DEVICE_NAME "uv"
diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
index 1f90976293e8..667d573e54b0 100644
--- a/drivers/s390/char/uvdevice.c
+++ b/drivers/s390/char/uvdevice.c
@@ -297,6 +297,43 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
 	return ret;
 }
 
+/*
+ * Do the actual secret list creation. Calls the list-UVC until there is no more
+ * space in the user buffer, or the list ends.
+ */
+static int uvio_get_list(void *zpage, struct uvio_ioctl_cb *uv_ioctl)
+{
+	const size_t data_off = offsetof(struct uv_secret_list, secrets);
+	u8 __user *user_buf = (u8 __user *)uv_ioctl->argument_addr;
+	struct uv_secret_list *list = zpage;
+	u16 num_secrets_stored = 0;
+	size_t user_off = data_off;
+	size_t copy_len;
+
+	do {
+		uv_list_secrets(list, list->next_secret_idx, &uv_ioctl->uv_rc,
+				&uv_ioctl->uv_rrc);
+		if (uv_ioctl->uv_rc != UVC_RC_EXECUTED &&
+		    uv_ioctl->uv_rc != UVC_RC_MORE_DATA)
+			break;
+
+		copy_len = sizeof(list->secrets[0]) * list->num_secr_stored;
+		WARN_ON(copy_len > sizeof(list->secrets));
+
+		if (copy_to_user(user_buf + user_off, list->secrets, copy_len))
+			return -EFAULT;
+
+		user_off += copy_len;
+		num_secrets_stored += list->num_secr_stored;
+	} while (uv_ioctl->uv_rc == UVC_RC_MORE_DATA &&
+		 user_off + sizeof(*list) <= uv_ioctl->argument_len);
+
+	list->num_secr_stored = num_secrets_stored;
+	if (copy_to_user(user_buf, list, data_off))
+		return -EFAULT;
+	return 0;
+}
+
 /** uvio_list_secrets() - perform a List Secret UVC
  * @uv_ioctl: ioctl control block
  *
@@ -308,6 +345,12 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
  *
  * The argument specifies the location for the result of the UV-Call.
  *
+ * Argument len must be a multiple of a page; 1-8 pages allowed.
+ * The list secrets IOCTL will call the list UVC multiple times and fill
+ * the provided user-buffer with list elements until either the list ends or
+ * the buffer is full. The list header is merged over all list header from the
+ * individual UVCs.
+ *
  * If the List Secrets 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.
@@ -318,31 +361,22 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
  */
 static int uvio_list_secrets(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_LIST_SECRETS,
-	};
-	void *secrets = NULL;
-	int ret = 0;
+	void *zpage = NULL;
+	int rc;
 
-	if (uv_ioctl->argument_len != UVIO_LIST_SECRETS_LEN)
+	if (uv_ioctl->argument_len == 0 ||
+	    uv_ioctl->argument_len % UVIO_LIST_SECRETS_LEN != 0 ||
+	    uv_ioctl->argument_len > UVIO_LIST_SECRETS_MAX_LEN)
 		return -EINVAL;
 
-	secrets = kvzalloc(UVIO_LIST_SECRETS_LEN, GFP_KERNEL);
-	if (!secrets)
+	zpage = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!zpage)
 		return -ENOMEM;
 
-	uvcb.addr = (u64)secrets;
-	uv_call_sched(0, (u64)&uvcb);
-	uv_ioctl->uv_rc = uvcb.header.rc;
-	uv_ioctl->uv_rrc = uvcb.header.rrc;
-
-	if (copy_to_user(user_buf_arg, secrets, UVIO_LIST_SECRETS_LEN))
-		ret = -EFAULT;
+	rc = uvio_get_list(zpage, uv_ioctl);
 
-	kvfree(secrets);
-	return ret;
+	free_page((unsigned long)zpage);
+	return rc;
 }
 
 /** uvio_lock_secrets() - perform a Lock Secret Store UVC
-- 
2.45.2
Re: [PATCH v2] s390/uvdevice: Support longer secret lists
Posted by Janosch Frank 2 weeks, 6 days ago
On 10/31/24 10:35 AM, Steffen Eiden wrote:
> Enable the list IOCTL to provide lists longer than on page (85 entries).

s/on/one/

> The list IOCTL accepts argument length up to 8 pages in page granularity
> and will fill the argument up to this length with entries until the list
> ends. User space unaware of this enhancement will still receive one page
> of data and an uv_rc 0x0100.

Once the length check is fixed I'll be happy with this patch.

> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
> Reworked the whole list-creation loop. Hardened+simplified the implementation.
> Now, only the actual data filled by the CP is copied to userspace.
> 
>   arch/s390/include/uapi/asm/uvdevice.h |  1 +
>   drivers/s390/char/uvdevice.c          | 72 ++++++++++++++++++++-------
>   2 files changed, 54 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
> index 4947f26ad9fb..c584250d4a35 100644
> --- a/arch/s390/include/uapi/asm/uvdevice.h
> +++ b/arch/s390/include/uapi/asm/uvdevice.h
> @@ -71,6 +71,7 @@ struct uvio_uvdev_info {
>   #define UVIO_ATT_ADDITIONAL_MAX_LEN	0x8000
>   #define UVIO_ADD_SECRET_MAX_LEN		0x100000
>   #define UVIO_LIST_SECRETS_LEN		0x1000
> +#define UVIO_LIST_SECRETS_MAX_LEN	0x8000

Since we're only ever allocating a page in the kernel it doesn't really 
make sense to arbitrarily limit this IMHO.

A check for 0 and page alignment should be enough.

>   #define UVIO_RETR_SECRET_MAX_LEN	0x2000
>   
>   #define UVIO_DEVICE_NAME "uv"
> diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
> index 1f90976293e8..667d573e54b0 100644
> --- a/drivers/s390/char/uvdevice.c
> +++ b/drivers/s390/char/uvdevice.c
> @@ -297,6 +297,43 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
>   	return ret;
>   }
>   
> +/*
> + * Do the actual secret list creation. Calls the list-UVC until there is no more

list secrets UVC

> + * space in the user buffer, or the list ends.
> + */
> +static int uvio_get_list(void *zpage, struct uvio_ioctl_cb *uv_ioctl)
> +{
> +	const size_t data_off = offsetof(struct uv_secret_list, secrets);
> +	u8 __user *user_buf = (u8 __user *)uv_ioctl->argument_addr;
> +	struct uv_secret_list *list = zpage;
> +	u16 num_secrets_stored = 0;
> +	size_t user_off = data_off;
> +	size_t copy_len;
> +
> +	do {
> +		uv_list_secrets(list, list->next_secret_idx, &uv_ioctl->uv_rc,
> +				&uv_ioctl->uv_rrc);
> +		if (uv_ioctl->uv_rc != UVC_RC_EXECUTED &&
> +		    uv_ioctl->uv_rc != UVC_RC_MORE_DATA)
> +			break;
> +
> +		copy_len = sizeof(list->secrets[0]) * list->num_secr_stored;
> +		WARN_ON(copy_len > sizeof(list->secrets));
> +
> +		if (copy_to_user(user_buf + user_off, list->secrets, copy_len))
> +			return -EFAULT;
> +
> +		user_off += copy_len;
> +		num_secrets_stored += list->num_secr_stored;
> +	} while (uv_ioctl->uv_rc == UVC_RC_MORE_DATA &&
> +		 user_off + sizeof(*list) <= uv_ioctl->argument_len);
> +
> +	list->num_secr_stored = num_secrets_stored;
> +	if (copy_to_user(user_buf, list, data_off))
> +		return -EFAULT;
> +	return 0;
> +}
> +
>   /** uvio_list_secrets() - perform a List Secret UVC
>    * @uv_ioctl: ioctl control block
>    *
> @@ -308,6 +345,12 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
>    *
>    * The argument specifies the location for the result of the UV-Call.
>    *
> + * Argument len must be a multiple of a page; 1-8 pages allowed.

Fix comment when adjusting len check.

> + * The list secrets IOCTL will call the list UVC multiple times and fill
> + * the provided user-buffer with list elements until either the list ends or
> + * the buffer is full. The list header is merged over all list header from the
> + * individual UVCs.
> + *