linux-kernel - Re: [PATCH v2] s390/uvdevice: Support longer secret lists

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <5bd1d878-69d0-4d27-9129-6fb8126ccb31@linux.ibm.com>
Date: Mon, 4 Nov 2024 13:13:16 +0100
From: Janosch Frank <frankja@...ux.ibm.com>
To: Steffen Eiden <seiden@...ux.ibm.com>, linux-kernel@...r.kernel.org,
        linux-s390@...r.kernel.org
Cc: Ingo Franzki <ifranzki@...ux.ibm.com>,
        Christoph Schlameuss <schlameuss@...ux.ibm.com>,
        Claudio Imbrenda <imbrenda@...ux.ibm.com>, borntraeger@...ux.ibm.com
Subject: Re: [PATCH v2] s390/uvdevice: Support longer secret lists

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@...ux.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.
> + *