| 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
| ||
|
Message-ID: <78aeaefa-495e-25ae-db8a-eae0394beb12@linux.ibm.com>
Date: Mon, 21 Feb 2022 12:08:41 +0100
From: Steffen Eiden <seiden@...ux.ibm.com>
To: Greg KH <greg@...ah.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
Hey Greg,
thanks for your review.
On 2/17/22 13:33, Greg KH wrote:
> 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.
These ints are unsigned, your szenario cannot happen. However, I changed
it to bailout early anyway.
>
>
>
>> +
>> + 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.
Makes sense.
>
>> +
>> +/*
>> + * 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 :(
changed
>
>> + 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?
We do not have anything like a s390 facility bus which could trigger
such automatic loads. Developing such a bus would be an overkill.
However we could do the approach e.g. kvm-s390 takes. Define
MODULE_ALIAS(devname:uv) that will trigger an automatic module load if
someone calls open on /dev/uv the first time.
IIRC we need to define a fixed misc minor number with this approach.
something like that:
--- a/drivers/s390/char/uvdevice.c
+++ b/drivers/s390/char/uvdevice.c
@@ -309,3 +309,5
MODULE_AUTHOR("IBM Corporation");
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Ultravisor UAPI driver");
+MODULE_ALIAS_MISCDEV(SOME_FIXED_MISC_MINOR);
+MODULE_ALIAS("devname:uv");
We then maybe need to discuss if 'uv' is unique enough.
>
> thanks,
>
> greg k-h
>
Steffen
Powered by blists - more mailing lists