Re: [PATCH v2 3/6] s390/uvdevice: Add 'List Secrets' UVC

[Janosch Frank <frankja@linux.ibm.com>]

On 5/19/23 11:37, Steffen Eiden wrote:
> Userspace can call the List Secrets Ultravisor Call
> using IOCTLs on the uvdevice.
> During the handling of the new IOCTL nr the uvdevice will do some sanity
> checks first. Then, perform the Ultravisor command, and copy the answer
> to userspace.
> 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. This is also true for any other
> possible error code the UV can return.
> 
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>[...]
> +/** uvio_list_secrets() - perform a List Secret UVC
> + *
> + * @uv_ioctl: ioctl control block
> + *
> + * uvio_list_secrets() performs the List 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 builds the request, performs the UV-call,
> + * and copies the result to userspace.
> + *
> + * The argument specifies the location for the result of the UV-Call.
> + *
> + * 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.
> + *
> + * Context: might sleep
> + *
> + * Return: 0 on success or a negative error code on error.
> + */
> +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;

int ret = 0;

> +
> +	if (uv_ioctl->argument_len != UVIO_LIST_SECRETS_LEN)
> +		return -EINVAL;

I'd be less uneasy if you 
s/uv_ioctl->argument_len/UVIO_LIST_SECRETS_LEN/ below. Yes, you check 
the length above but it still feels weird to use a variable when we have 
a perfectly fine constant just waiting to be used.

> +
> +	secrets = kvzalloc(uv_ioctl->argument_len, GFP_KERNEL);
> +	if (!secrets)
> +		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, uv_ioctl->argument_len))
> +		ret = -EFAULT;

and remove the else

> +	else
> +		ret = 0;
> +
> +	kvfree(secrets);
> +	return ret;
> +}
> +
>   static int uvio_copy_and_check_ioctl(struct uvio_ioctl_cb *ioctl, void __user *argp,
>   				     unsigned long cmd)
>   {
> @@ -333,6 +385,9 @@ static long uvio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	case UVIO_IOCTL_ADD_SECRET_NR:
>   		ret = uvio_add_secret(&uv_ioctl);
>   		break;
> +	case UVIO_IOCTL_LIST_SECRETS_NR:
> +		ret = uvio_list_secrets(&uv_ioctl);
> +		break;
>   	default:
>   		ret = -ENOIOCTLCMD;
>   		break;