Re: [PATCH 1/3] drivers/s390/char: Add Ultravisor io device

[Greg KH]

On Thu, Feb 17, 2022 at 06:37:15AM -0500, Steffen Eiden wrote:
> This patch adds a new miscdevice to expose some Ultravisor functions
> to userspace. Userspace can send IOCTLis to the uvdevice that will then
> emit a corresponding Ultravisor Call and hands the result over to
> userspace. The uvdevice is available if the Ultravisor Call facility is
> present.
> 
> Userspace is now able to call the Query Ultravisor Information
> Ultravisor Command through the uvdevice.
> 
> Signed-off-by: Steffen Eiden <seiden@xxxxxxxxxxxxx>
> ---
>  MAINTAINERS                           |   2 +
>  arch/s390/include/uapi/asm/uvdevice.h |  27 +++++
>  drivers/s390/char/Kconfig             |   9 ++
>  drivers/s390/char/Makefile            |   1 +
>  drivers/s390/char/uvdevice.c          | 162 ++++++++++++++++++++++++++
>  5 files changed, 201 insertions(+)
>  create mode 100644 arch/s390/include/uapi/asm/uvdevice.h
>  create mode 100644 drivers/s390/char/uvdevice.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5250298d2817..c7d8d0fe48cf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10457,9 +10457,11 @@ F:	Documentation/virt/kvm/s390*
>  F:	arch/s390/include/asm/gmap.h
>  F:	arch/s390/include/asm/kvm*
>  F:	arch/s390/include/uapi/asm/kvm*
> +F:	arch/s390/include/uapi/asm/uvdevice.h
>  F:	arch/s390/kernel/uv.c
>  F:	arch/s390/kvm/
>  F:	arch/s390/mm/gmap.c
> +F:	drivers/s390/char/uvdevice.c
>  F:	tools/testing/selftests/kvm/*/s390x/
>  F:	tools/testing/selftests/kvm/s390x/
>  
> diff --git a/arch/s390/include/uapi/asm/uvdevice.h b/arch/s390/include/uapi/asm/uvdevice.h
> new file mode 100644
> index 000000000000..f2e4984a6e2e
> --- /dev/null
> +++ b/arch/s390/include/uapi/asm/uvdevice.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + *  Copyright IBM Corp. 2022
> + *  Author(s): Steffen Eiden <seiden@xxxxxxxxxxxxx>
> + */
> +#ifndef __S390X_ASM_UVDEVICE_H
> +#define __S390X_ASM_UVDEVICE_H
> +
> +#include <linux/types.h>
> +
> +struct uvio_ioctl_cb {
> +	__u32 flags;			/* Currently no flags defined, must be zero */
> +	__u16 uv_rc;			/* UV header rc value */
> +	__u16 uv_rrc;			/* UV header rrc value */
> +	__u64 argument_addr;		/* Userspace address of uvio argument */
> +	__u32 argument_len;
> +	__u8  reserved14[0x40 - 0x14];	/* must be zero */
> +};
> +
> +#define UVIO_QUI_MAX_LEN		0x8000
> +
> +#define UVIO_DEVICE_NAME "uv"
> +#define UVIO_TYPE_UVC 'u'
> +
> +#define UVIO_IOCTL_QUI _IOWR(UVIO_TYPE_UVC, 0x01, struct uvio_ioctl_cb)
> +
> +#endif  /* __S390X_ASM_UVDEVICE_H */
> diff --git a/drivers/s390/char/Kconfig b/drivers/s390/char/Kconfig
> index 6cc4b19acf85..933c0d0062d6 100644
> --- a/drivers/s390/char/Kconfig
> +++ b/drivers/s390/char/Kconfig
> @@ -184,3 +184,12 @@ config S390_VMUR
>  	depends on S390
>  	help
>  	  Character device driver for z/VM reader, puncher and printer.
> +
> +config UV_UAPI
> +	def_tristate m
> +	prompt "Ultravisor userspace API"
> +	depends on PROTECTED_VIRTUALIZATION_GUEST
> +	help
> +	  Selecting exposes parts of the UV interface to userspace
> +	  by providing a misc character device. Using IOCTLs one
> +	  can interact with the UV.
> diff --git a/drivers/s390/char/Makefile b/drivers/s390/char/Makefile
> index c6fdb81a068a..b5c83092210e 100644
> --- a/drivers/s390/char/Makefile
> +++ b/drivers/s390/char/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_MONREADER) += monreader.o
>  obj-$(CONFIG_MONWRITER) += monwriter.o
>  obj-$(CONFIG_S390_VMUR) += vmur.o
>  obj-$(CONFIG_CRASH_DUMP) += sclp_sdias.o zcore.o
> +obj-$(CONFIG_UV_UAPI) += uvdevice.o
>  
>  hmcdrv-objs := hmcdrv_mod.o hmcdrv_dev.o hmcdrv_ftp.o hmcdrv_cache.o diag_ftp.o sclp_ftp.o
>  obj-$(CONFIG_HMC_DRV) += hmcdrv.o
> diff --git a/drivers/s390/char/uvdevice.c b/drivers/s390/char/uvdevice.c
> new file mode 100644
> index 000000000000..e8efcbf0e7ab
> --- /dev/null
> +++ b/drivers/s390/char/uvdevice.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *  Copyright IBM Corp. 2022
> + *  Author(s): Steffen Eiden <seiden@xxxxxxxxxxxxx>
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt ".\n"
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/types.h>
> +#include <linux/stddef.h>
> +#include <linux/vmalloc.h>
> +#include <linux/slab.h>
> +
> +#include <asm/uvdevice.h>
> +#include <asm/uv.h>
> +
> +/**
> + * uvio_qui() - Perform a Query Ultravisor Information UVC.
> + *
> + * uv_ioctl: ioctl control block
> + *
> + * uvio_qui() does a Query Ultravisor Information (QUI) Ultravisor Call.
> + * It creates the uvc qui request and sends it to the Ultravisor. After that
> + * it copies the response to userspace and fills the rc and rrc of uv_ioctl
> + * uv_call with the response values of the Ultravisor.
> + *
> + * Create the UVC structure, send the UVC to UV and write the response in the ioctl struct.
> + *
> + * Return: 0 on success or a negative error code on error.
> + */
> +static int uvio_qui(struct uvio_ioctl_cb *uv_ioctl)
> +{
> +	u8 __user *user_buf_addr = (__user u8 *)uv_ioctl->argument_addr;
> +	size_t user_buf_len = uv_ioctl->argument_len;
> +	struct uv_cb_header *uvcb_qui = NULL;
> +	int ret;
> +
> +	/*
> +	 * Do not check for a too small buffer. If userspace provides a buffer
> +	 * that is too small the Ultravisor will complain.
> +	 */
> +	ret = -EINVAL;
> +	if (!user_buf_len || user_buf_len > UVIO_QUI_MAX_LEN)
> +		goto out;
> +	ret = -ENOMEM;
> +	uvcb_qui = kvzalloc(user_buf_len, GFP_KERNEL);
> +	if (!uvcb_qui)
> +		goto out;
> +	uvcb_qui->len = user_buf_len;
> +	uvcb_qui->cmd = UVC_CMD_QUI;
> +
> +	uv_call(0, (u64)uvcb_qui);
> +
> +	ret = -EFAULT;
> +	if (copy_to_user(user_buf_addr, uvcb_qui, uvcb_qui->len))
> +		goto out;
> +	uv_ioctl->uv_rc = uvcb_qui->rc;
> +	uv_ioctl->uv_rrc = uvcb_qui->rrc;
> +
> +	ret = 0;
> +out:
> +	kvfree(uvcb_qui);
> +	return ret;
> +}
> +
> +static int uvio_copy_and_check_ioctl(struct uvio_ioctl_cb *ioctl, void __user *argp)
> +{
> +	u64 sum = 0;
> +	u64 i;
> +
> +	if (copy_from_user(ioctl, argp, sizeof(*ioctl)))
> +		return -EFAULT;
> +	if (ioctl->flags != 0)
> +		return -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(ioctl->reserved14); i++)
> +		sum += ioctl->reserved14[i];
> +	if (sum)
> +		return -EINVAL;

So you can have -1, 1, -1, 1, and so on and cause this to be an
incorrect check.  Just test for 0 and bail out early please.



> +
> +	return 0;
> +}
> +
> +static int uvio_dev_open(struct inode *inode, struct file *filp)
> +{
> +	return 0;
> +}
> +
> +static int uvio_dev_close(struct inode *inodep, struct file *filp)
> +{
> +	return 0;
> +}

If open/close do nothing, no need to provide it at all, just drop them.

> +
> +/*
> + * IOCTL entry point for the Ultravisor device.
> + */
> +static long uvio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	struct uvio_ioctl_cb *uv_ioctl;
> +	long ret;
> +
> +	ret = -ENOMEM;
> +	uv_ioctl = vzalloc(sizeof(*uv_ioctl));
> +	if (!uv_ioctl)
> +		goto out;
> +
> +	switch (cmd) {
> +	case UVIO_IOCTL_QUI:
> +		ret = uvio_copy_and_check_ioctl(uv_ioctl, argp);
> +		if (ret)
> +			goto out;
> +		ret = uvio_qui(uv_ioctl);
> +		break;
> +	default:
> +		ret = -EINVAL;

Wrong error value :(

> +		break;
> +	}
> +	if (ret)
> +		goto out;
> +
> +	if (copy_to_user(argp, uv_ioctl, sizeof(*uv_ioctl)))
> +		ret = -EFAULT;
> +
> + out:
> +	vfree(uv_ioctl);
> +	return ret;
> +}
> +
> +static const struct file_operations uvio_dev_fops = {
> +	.owner = THIS_MODULE,
> +	.unlocked_ioctl = uvio_ioctl,
> +	.open = uvio_dev_open,
> +	.release = uvio_dev_close,
> +	.llseek = no_llseek,
> +};
> +
> +static struct miscdevice uvio_dev_miscdev = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = UVIO_DEVICE_NAME,
> +	.fops = &uvio_dev_fops,
> +};
> +
> +static void __exit uvio_dev_exit(void)
> +{
> +	misc_deregister(&uvio_dev_miscdev);
> +}
> +
> +static int __init uvio_dev_init(void)
> +{
> +	if (!test_facility(158))
> +		return -ENXIO;
> +	return misc_register(&uvio_dev_miscdev);
> +}
> +
> +module_init(uvio_dev_init);
> +module_exit(uvio_dev_exit);
> +
> +MODULE_AUTHOR("IBM Corporation");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Ultravisor UAPI driver");

Nothing to cause this to automatically be loaded when the "hardware" is
present?

thanks,

greg k-h