| 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
| ||
|
Date: Thu, 17 Feb 2022 13:33:13 +0100
From: Greg KH <greg@...ah.com>
To: Steffen Eiden <seiden@...ux.ibm.com>
Cc: Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
David Hildenbrand <david@...hat.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>,
Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
linux-s390@...r.kernel.org, kvm@...r.kernel.org,
linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 1/3] drivers/s390/char: Add Ultravisor io device
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@...ux.ibm.com>
> ---
> 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@...ux.ibm.com>
> + */
> +#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@...ux.ibm.com>
> + */
> +#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
Powered by blists - more mailing lists