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

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

[Steffen Eiden]

Enable the list IOCTL to provide lists longer than one page (85 entries).
The list IOCTL now accepts any argument length in page granularity.
It fills 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>
---
 v3: remove upper boundary (8 pages) for arg len

 drivers/s390/char/uvdevice.c | 71 ++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
index 1f90976293e8..7551b03d5f99 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 secrets 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 length must be a multiple of a page.
+ * 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,21 @@ 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)
 		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 v3] s390/uvdevice: Support longer secret lists

[Janosch Frank]

On 11/4/24 4:36 PM, Steffen Eiden wrote:
> Enable the list IOCTL to provide lists longer than one page (85 entries).
> The list IOCTL now accepts any argument length in page granularity.
> It fills 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.
>

Thanks for working in all of the changes that I requested:
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

Re: [PATCH v3] s390/uvdevice: Support longer secret lists

[Christoph Schlameuss]

LGTM

On Mon Nov 4, 2024 at 4:36 PM CET, Steffen Eiden wrote:
> Enable the list IOCTL to provide lists longer than one page (85 entries).
> The list IOCTL now accepts any argument length in page granularity.
> It fills 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>

Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>

> ---
>  v3: remove upper boundary (8 pages) for arg len
>
>  drivers/s390/char/uvdevice.c | 71 ++++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 19 deletions(-)

[...]

Re: [PATCH v3] s390/uvdevice: Support longer secret lists

[Heiko Carstens]

On Mon, Nov 04, 2024 at 04:36:09PM +0100, Steffen Eiden wrote:
> Enable the list IOCTL to provide lists longer than one page (85 entries).
> The list IOCTL now accepts any argument length in page granularity.
> It fills 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>
> ---
>  v3: remove upper boundary (8 pages) for arg len

...

> +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));

Is this really possible? Without checking the documentation I guess
this is not possible and therefore the WARN_ON() should be removed.

If however this can be possible then this should be turned into a
WARN_ON_ONCE().

> +		if (copy_to_user(user_buf + user_off, list->secrets, copy_len))
> +			return -EFAULT;

...and in addition, if the above would be possible this _could_ copy
random kernel data to user space. Not good.

Re: [PATCH v3] s390/uvdevice: Support longer secret lists

[Janosch Frank]

On 11/6/24 9:10 AM, Heiko Carstens wrote:
> On Mon, Nov 04, 2024 at 04:36:09PM +0100, Steffen Eiden wrote:
>> Enable the list IOCTL to provide lists longer than one page (85 entries).
>> The list IOCTL now accepts any argument length in page granularity.
>> It fills 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>
>> ---
>>   v3: remove upper boundary (8 pages) for arg len
> 
> ...
> 
>> +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));
> 
> Is this really possible? Without checking the documentation I guess
> this is not possible and therefore the WARN_ON() should be removed.
> 

This happening requires a FW error, no?
list->num_secr_stored is reported by FW and would need to be >85.

We could clamp it down to 85 secrets / 4k - sizeof(header) with a 
WARN_ON_ONCE() to catch FW problems if that suits you more.

Re: [PATCH v3] s390/uvdevice: Support longer secret lists

[Heiko Carstens]

On Wed, Nov 06, 2024 at 09:54:33AM +0100, Janosch Frank wrote:
> On 11/6/24 9:10 AM, Heiko Carstens wrote:
> > On Mon, Nov 04, 2024 at 04:36:09PM +0100, Steffen Eiden wrote:
> > > +		copy_len = sizeof(list->secrets[0]) * list->num_secr_stored;
> > > +		WARN_ON(copy_len > sizeof(list->secrets));
> > 
> > Is this really possible? Without checking the documentation I guess
> > this is not possible and therefore the WARN_ON() should be removed.
> > 
> 
> This happening requires a FW error, no?
> list->num_secr_stored is reported by FW and would need to be >85.
> 
> We could clamp it down to 85 secrets / 4k - sizeof(header) with a
> WARN_ON_ONCE() to catch FW problems if that suits you more.

If this would be an *error* why even add this check? We have tons of
code without doing sanity checks for firmware provided values - where
should we start or end?

So imho: either remove this check if this would be firmware error,
unless there is a good reason do keep this check, or if this is not an
error convert to WARN_ON_ONCE() and limit the copy_to_user().

Re: [PATCH v3] s390/uvdevice: Support longer secret lists

[Steffen Eiden]

On 11/6/24 10:13 AM, Heiko Carstens wrote:
> On Wed, Nov 06, 2024 at 09:54:33AM +0100, Janosch Frank wrote:
>> On 11/6/24 9:10 AM, Heiko Carstens wrote:
>>> On Mon, Nov 04, 2024 at 04:36:09PM +0100, Steffen Eiden wrote:
>>>> +		copy_len = sizeof(list->secrets[0]) * list->num_secr_stored;
>>>> +		WARN_ON(copy_len > sizeof(list->secrets));
>>>
>>> Is this really possible? Without checking the documentation I guess
>>> this is not possible and therefore the WARN_ON() should be removed.
>>>
>>
>> This happening requires a FW error, no?
>> list->num_secr_stored is reported by FW and would need to be >85.
>>
>> We could clamp it down to 85 secrets / 4k - sizeof(header) with a
>> WARN_ON_ONCE() to catch FW problems if that suits you more.
> 
> If this would be an *error* why even add this check? We have tons of
> code without doing sanity checks for firmware provided values - where
> should we start or end?
Yes, this would be an error.

> 
> So imho: either remove this check if this would be firmware error,
> unless there is a good reason do keep this check, or if this is not an
> error convert to WARN_ON_ONCE() and limit the copy_to_user().
OK. I'll remove the check. Sending a fix-up reply.

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

[Steffen Eiden]

Enable the list IOCTL to provide lists longer than one page (85 entries).
The list IOCTL now accepts any argument length in page granularity.
It fills 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.

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Christoph Schlameuss <schlameuss@linux.ibm.com>
Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
---
v4: add r-b, remove WARN

 drivers/s390/char/uvdevice.c | 69 ++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 19 deletions(-)

diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
index 1f90976293e8..c5c8dd13590a 100644
--- a/drivers/s390/char/uvdevice.c
+++ b/drivers/s390/char/uvdevice.c
@@ -297,6 +297,41 @@ static int uvio_add_secret(struct uvio_ioctl_cb *uv_ioctl)
 	return ret;
 }
 
+/*
+ * Do the actual secret list creation. Calls the list secrets 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;
+		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 +343,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 length must be a multiple of a page.
+ * 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 +359,21 @@ 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)
 		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