Re: [PATCH v2 8/9] platform/surface: Add Surface Aggregator user-space interface

Hans de Goede
2020-12-15T16:36:57Z

Hi,

On 12/3/20 10:26 PM, Maximilian Luz wrote:
> Add a misc-device providing user-space access to the Surface Aggregator
> EC, mainly intended for debugging, testing, and reverse-engineering.
> This interface gives user-space applications the ability to send
> requests to the EC and receive the corresponding responses.
>
> The device-file is managed by a pseudo platform-device and corresponding
> driver to avoid dependence on the dedicated bus, allowing it to be
> loaded in a minimal configuration.
>
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>

1 review comment inline:

> ---
>
> Changes in v1 (from RFC):
> - add copyright lines
> - change SPDX identifier to GPL-2.0+ (was GPL-2.0-or-later)
> - move from debugfs to proper misc-device
> - move interface definitions to uapi header
> - remove versioning
> - allocate platform device dynamically
> - prioritize EFAULT error in IOCTL return
> - use ENOTTY error for unknown IOCTLs
> - fix IOCTL compatibility for 32-bit user-space on 64-bit kernels
> - mark request struct as __attribute__((__packed__))
> - address issues when driver is unbound while file still open
>
> Changes in v2:
> - add ioctl-number.rst entry
> - improve documentation
> - spell check comments and strings, fix typos
> - unify comment style
> - run checkpatch --strict, fix warnings and style issues
>
> ---
> .../surface_aggregator/clients/cdev.rst | 85 +++++
> .../surface_aggregator/clients/index.rst | 12 +-
> .../userspace-api/ioctl/ioctl-number.rst | 2 +
> MAINTAINERS | 2 +
> drivers/platform/surface/Kconfig | 17 +
> drivers/platform/surface/Makefile | 1 +
> .../surface/surface_aggregator_cdev.c | 299 ++++++++++++++++++
> include/uapi/linux/surface_aggregator/cdev.h | 58 ++++
> 8 files changed, 475 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/driver-api/surface_aggregator/clients/cdev.rst
> create mode 100644 drivers/platform/surface/surface_aggregator_cdev.c
> create mode 100644 include/uapi/linux/surface_aggregator/cdev.h
>
> diff --git a/Documentation/driver-api/surface_aggregator/clients/cdev.rst b/Documentation/driver-api/surface_aggregator/clients/cdev.rst
> new file mode 100644
> index 000000000000..f30e77def838
> --- /dev/null
> +++ b/Documentation/driver-api/surface_aggregator/clients/cdev.rst
> @@ -0,0 +1,85 @@
> +.. SPDX-License-Identifier: GPL-2.0+
> +
> +.. |u8| replace:: :c:type:`u8 <u8>`
> +.. |u16| replace:: :c:type:`u16 <u16>`
> +.. |ssam_cdev_request| replace:: :c:type:`struct ssam_cdev_request <ssam_cdev_request>`
> +.. |ssam_request_flags| replace:: :c:type:`enum ssam_request_flags <ssam_request_flags>`
> +
> +==============================
> +User-Space EC Interface (cdev)
> +==============================
> +
> +The ``surface_aggregator_cdev`` module provides a misc-device for the SSAM
> +controller to allow for a (more or less) direct connection from user-space to
> +the SAM EC. It is intended to be used for development and debugging, and
> +therefore should not be used or relied upon in any other way. Note that this
> +module is not loaded automatically, but instead must be loaded manually.
> +
> +The provided interface is accessible through the ``/dev/surface/aggregator``
> +device-file. All functionality of this interface is provided via IOCTLs.
> +These IOCTLs and their respective input/output parameter structs are defined in
> +``include/uapi/linux/surface_aggregator/cdev.h``.
> +
> +
> +Controller IOCTLs
> +=================
> +
> +The following IOCTLs are provided:
> +
> +.. flat-table:: Controller IOCTLs
> + :widths: 1 1 1 1 4
> + :header-rows: 1
> +
> + * - Type
> + - Number
> + - Direction
> + - Name
> + - Description
> +
> + * - ``0xA5``
> + - ``1``
> + - ``WR``
> + - ``REQUEST``
> + - Perform synchronous SAM request.
> +
> +
> +``REQUEST``
> +-----------
> +
> +Defined as ``_IOWR(0xA5, 1, struct ssam_cdev_request)``.
> +
> +Executes a synchronous SAM request. The request specification is passed in
> +as argument of type |ssam_cdev_request|, which is then written to/modified
> +by the IOCTL to return status and result of the request.
> +
> +Request payload data must be allocated separately and is passed in via the
> +``payload.data`` and ``payload.length`` members. If a response is required,
> +the response buffer must be allocated by the caller and passed in via the
> +``response.data`` member. The ``response.length`` member must be set to the
> +capacity of this buffer, or if no response is required, zero. Upon
> +completion of the request, the call will write the response to the response
> +buffer (if its capacity allows it) and overwrite the length field with the
> +actual size of the response, in bytes.
> +
> +Additionally, if the request has a response, this must be indicated via the
> +request flags, as is done with in-kernel requests. Request flags can be set
> +via the ``flags`` member and the values correspond to the values found in
> +|ssam_request_flags|.
> +
> +Finally, the status of the request itself is returned in the ``status``
> +member (a negative errno value indicating failure). Note that failure
> +indication of the IOCTL is separated from failure indication of the request:
> +The IOCTL returns a negative status code if anything failed during setup of
> +the request (``-EFAULT``) or if the provided argument or any of its fields
> +are invalid (``-EINVAL``). In this case, the status value of the request
> +argument may be set, providing more detail on what went wrong (e.g.
> +``-ENOMEM`` for out-of-memory), but this value may also be zero. The IOCTL
> +will return with a zero status code in case the request has been set up,
> +submitted, and completed (i.e. handed back to user-space) successfully from
> +inside the IOCTL, but the request ``status`` member may still be negative in
> +case the actual execution of the request failed after it has been submitted.
> +
> +A full definition of the argument struct is provided below:
> +
> +.. kernel-doc:: include/uapi/linux/surface_aggregator/cdev.h
> + :functions: ssam_cdev_request
> diff --git a/Documentation/driver-api/surface_aggregator/clients/index.rst b/Documentation/driver-api/surface_aggregator/clients/index.rst
> index 31e026d96102..ab260ec82cfb 100644
> --- a/Documentation/driver-api/surface_aggregator/clients/index.rst
> +++ b/Documentation/driver-api/surface_aggregator/clients/index.rst
> @@ -7,4 +7,14 @@ Client Driver Documentation
> This is the documentation for client drivers themselves. Refer to
> :doc:`../client` for documentation on how to write client drivers.
>
> -.. Place documentation for individual client drivers here.
> +.. toctree::
> + :maxdepth: 1
> +
> + cdev
> +
> +.. only:: subproject and html
> +
> + Indices
> + =======
> +
> + * :ref:`genindex`
> diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
> index 55a2d9b2ce33..a98895da62b3 100644
> --- a/Documentation/userspace-api/ioctl/ioctl-number.rst
> +++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
> @@ -323,6 +323,8 @@ Code Seq# Include File Comments
> <mailto:tlewis@mindspring.com>
> 0xA3 90-9F linux/dtlk.h
> 0xA4 00-1F uapi/linux/tee.h Generic TEE subsystem
> +0xA5 01 linux/surface_aggregator/cdev.h Microsoft Surface Platform System Aggregator
> + <mailto:luzmaximilian@gmail.com>
> 0xAA 00-3F linux/uapi/linux/userfaultfd.h
> 0xAB 00-1F linux/nbd.h
> 0xAC 00-1F linux/raw.h
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ff5d60875cec..f5a788f445a4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11699,7 +11699,9 @@ W: https://github.com/linux-surface/surface-aggregator-module
> C: irc://chat.freenode.net/##linux-surface
> F: Documentation/driver-api/surface_aggregator/
> F: drivers/platform/surface/aggregator/
> +F: drivers/platform/surface/surface_aggregator_cdev.c
> F: include/linux/surface_aggregator/
> +F: include/uapi/linux/surface_aggregator/
>
> MICROTEK X6 SCANNER
> M: Oliver Neukum <oliver@neukum.org>
> diff --git a/drivers/platform/surface/Kconfig b/drivers/platform/surface/Kconfig
> index 5af22f5141ff..cecad7a0cb7b 100644
> --- a/drivers/platform/surface/Kconfig
> +++ b/drivers/platform/surface/Kconfig
> @@ -40,6 +40,23 @@ config SURFACE_3_POWER_OPREGION
> This driver provides support for ACPI operation
> region of the Surface 3 battery platform driver.
>
> +config SURFACE_AGGREGATOR_CDEV
> + tristate "Surface System Aggregator Module User-Space Interface"
> + depends on SURFACE_AGGREGATOR
> + help
> + Provides a misc-device interface to the Surface System Aggregator
> + Module (SSAM) controller.
> +
> + This option provides a module (called surface_aggregator_cdev), that,
> + when loaded, will add a client device (and its respective driver) to
> + the SSAM controller. Said client device manages a misc-device
> + interface (/dev/surface/aggregator), which can be used by user-space
> + tools to directly communicate with the SSAM EC by sending requests and
> + receiving the corresponding responses.
> +
> + The provided interface is intended for debugging and development only,
> + and should not be used otherwise.
> +
> config SURFACE_GPE
> tristate "Surface GPE/Lid Support Driver"
> depends on ACPI
> diff --git a/drivers/platform/surface/Makefile b/drivers/platform/surface/Makefile
> index 034134fe0264..161f0ad05795 100644
> --- a/drivers/platform/surface/Makefile
> +++ b/drivers/platform/surface/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_SURFACE3_WMI) += surface3-wmi.o
> obj-$(CONFIG_SURFACE_3_BUTTON) += surface3_button.o
> obj-$(CONFIG_SURFACE_3_POWER_OPREGION) += surface3_power.o
> obj-$(CONFIG_SURFACE_AGGREGATOR) += aggregator/
> +obj-$(CONFIG_SURFACE_AGGREGATOR_CDEV) += surface_aggregator_cdev.o
> obj-$(CONFIG_SURFACE_GPE) += surface_gpe.o
> obj-$(CONFIG_SURFACE_PRO3_BUTTON) += surfacepro3_button.o
> diff --git a/drivers/platform/surface/surface_aggregator_cdev.c b/drivers/platform/surface/surface_aggregator_cdev.c
> new file mode 100644
> index 000000000000..40c30627f33a
> --- /dev/null
> +++ b/drivers/platform/surface/surface_aggregator_cdev.c
> @@ -0,0 +1,299 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Provides user-space access to the SSAM EC via the /dev/surface/aggregator
> + * misc device. Intended for debugging and development.
> + *
> + * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/kref.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/rwsem.h>
> +#include <linux/slab.h>
> +#include <linux/uaccess.h>
> +
> +#include <linux/surface_aggregator/cdev.h>
> +#include <linux/surface_aggregator/controller.h>
> +
> +#define SSAM_CDEV_DEVICE_NAME "surface_aggregator_cdev"
> +
> +struct ssam_cdev {
> + struct kref kref;
> + struct rw_semaphore lock;
> + struct ssam_controller *ctrl;
> + struct miscdevice mdev;
> +};
> +
> +static void __ssam_cdev_release(struct kref *kref)
> +{
> + kfree(container_of(kref, struct ssam_cdev, kref));
> +}
> +
> +static struct ssam_cdev *ssam_cdev_get(struct ssam_cdev *cdev)
> +{
> + if (cdev)
> + kref_get(&cdev->kref);
> +
> + return cdev;
> +}
> +
> +static void ssam_cdev_put(struct ssam_cdev *cdev)
> +{
> + if (cdev)
> + kref_put(&cdev->kref, __ssam_cdev_release);
> +}
> +
> +static int ssam_cdev_device_open(struct inode *inode, struct file *filp)
> +{
> + struct miscdevice *mdev = filp->private_data;
> + struct ssam_cdev *cdev = container_of(mdev, struct ssam_cdev, mdev);
> +
> + filp->private_data = ssam_cdev_get(cdev);
> + return stream_open(inode, filp);
> +}
> +
> +static int ssam_cdev_device_release(struct inode *inode, struct file *filp)
> +{
> + ssam_cdev_put(filp->private_data);
> + return 0;
> +}
> +
> +static long ssam_cdev_request(struct ssam_cdev *cdev, unsigned long arg)
> +{
> + struct ssam_cdev_request __user *r;
> + struct ssam_cdev_request rqst;
> + struct ssam_request spec;
> + struct ssam_response rsp;
> + const void __user *plddata;
> + void __user *rspdata;
> + int status = 0, ret = 0, tmp;
> +
> + r = (struct ssam_cdev_request __user *)arg;
> + ret = copy_struct_from_user(&rqst, sizeof(rqst), r, sizeof(*r));
> + if (ret)
> + goto out;
> +
> + plddata = u64_to_user_ptr(rqst.payload.data);
> + rspdata = u64_to_user_ptr(rqst.response.data);
> +
> + /* Setup basic request fields. */
> + spec.target_category = rqst.target_category;
> + spec.target_id = rqst.target_id;
> + spec.command_id = rqst.command_id;
> + spec.instance_id = rqst.instance_id;
> + spec.flags = rqst.flags;
> + spec.length = rqst.payload.length;
> + spec.payload = NULL;
> +
> + rsp.capacity = rqst.response.length;
> + rsp.length = 0;
> + rsp.pointer = NULL;
> +
> + /* Get request payload from user-space. */
> + if (spec.length) {
> + if (!plddata) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + spec.payload = kzalloc(spec.length, GFP_KERNEL);
> + if (!spec.payload) {
> + status = -ENOMEM;
> + ret = -EFAULT;
> + goto out;
> + }
> +
> + if (copy_from_user((void *)spec.payload, plddata, spec.length)) {
> + ret = -EFAULT;
> + goto out;
> + }
> + }
> +
> + /* Allocate response buffer. */
> + if (rsp.capacity) {
> + if (!rspdata) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + rsp.pointer = kzalloc(rsp.capacity, GFP_KERNEL);
> + if (!rsp.pointer) {
> + status = -ENOMEM;
> + ret = -EFAULT;

This is weird, -EFAULT should only be used if a SEGFAULT
would have been raised if the code was running in
userspace rather then in kernelspace, IOW if userspace
has provided an invalid pointer (or a too small buffer,
causing the pointer to become invalid at some point in
the buffer).

IMHO you should simply do ret = -ENOMEM here.

Otherwise this looks good to me.

Regards,

Hans

> + goto out;
> + }
> + }
> +
> + /* Perform request. */
> + status = ssam_request_sync(cdev->ctrl, &spec, &rsp);
> + if (status)
> + goto out;
> +
> + /* Copy response to user-space. */
> + if (rsp.length && copy_to_user(rspdata, rsp.pointer, rsp.length))
> + ret = -EFAULT;
> +
> +out:
> + /* Always try to set response-length and status. */
> + tmp = put_user(rsp.length, &r->response.length);
> + if (tmp)
> + ret = tmp;
> +
> + tmp = put_user(status, &r->status);
> + if (tmp)
> + ret = tmp;
> +
> + /* Cleanup. */
> + kfree(spec.payload);
> + kfree(rsp.pointer);
> +
> + return ret;
> +}
> +
> +static long __ssam_cdev_device_ioctl(struct ssam_cdev *cdev, unsigned int cmd,
> + unsigned long arg)
> +{
> + switch (cmd) {
> + case SSAM_CDEV_REQUEST:
> + return ssam_cdev_request(cdev, arg);
> +
> + default:
> + return -ENOTTY;
> + }
> +}
> +
> +static long ssam_cdev_device_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct ssam_cdev *cdev = file->private_data;
> + long status;
> +
> + /* Ensure that controller is valid for as long as we need it. */
> + if (down_read_killable(&cdev->lock))
> + return -ERESTARTSYS;
> +
> + if (!cdev->ctrl) {
> + up_read(&cdev->lock);
> + return -ENODEV;
> + }
> +
> + status = __ssam_cdev_device_ioctl(cdev, cmd, arg);
> +
> + up_read(&cdev->lock);
> + return status;
> +}
> +
> +static const struct file_operations ssam_controller_fops = {
> + .owner = THIS_MODULE,
> + .open = ssam_cdev_device_open,
> + .release = ssam_cdev_device_release,
> + .unlocked_ioctl = ssam_cdev_device_ioctl,
> + .compat_ioctl = ssam_cdev_device_ioctl,
> + .llseek = noop_llseek,
> +};
> +
> +static int ssam_dbg_device_probe(struct platform_device *pdev)
> +{
> + struct ssam_controller *ctrl;
> + struct ssam_cdev *cdev;
> + int status;
> +
> + ctrl = ssam_client_bind(&pdev->dev);
> + if (IS_ERR(ctrl))
> + return PTR_ERR(ctrl) == -ENODEV ? -EPROBE_DEFER : PTR_ERR(ctrl);
> +
> + cdev = kzalloc(sizeof(*cdev), GFP_KERNEL);
> + if (!cdev)
> + return -ENOMEM;
> +
> + kref_init(&cdev->kref);
> + init_rwsem(&cdev->lock);
> + cdev->ctrl = ctrl;
> +
> + cdev->mdev.parent = &pdev->dev;
> + cdev->mdev.minor = MISC_DYNAMIC_MINOR;
> + cdev->mdev.name = "surface_aggregator";
> + cdev->mdev.nodename = "surface/aggregator";
> + cdev->mdev.fops = &ssam_controller_fops;
> +
> + status = misc_register(&cdev->mdev);
> + if (status) {
> + kfree(cdev);
> + return status;
> + }
> +
> + platform_set_drvdata(pdev, cdev);
> + return 0;
> +}
> +
> +static int ssam_dbg_device_remove(struct platform_device *pdev)
> +{
> + struct ssam_cdev *cdev = platform_get_drvdata(pdev);
> +
> + misc_deregister(&cdev->mdev);
> +
> + /*
> + * The controller is only guaranteed to be valid for as long as the
> + * driver is bound. Remove controller so that any lingering open files
> + * cannot access it any more after we're gone.
> + */
> + down_write(&cdev->lock);
> + cdev->ctrl = NULL;
> + up_write(&cdev->lock);
> +
> + ssam_cdev_put(cdev);
> + return 0;
> +}
> +
> +static struct platform_device *ssam_cdev_device;
> +
> +static struct platform_driver ssam_cdev_driver = {
> + .probe = ssam_dbg_device_probe,
> + .remove = ssam_dbg_device_remove,
> + .driver = {
> + .name = SSAM_CDEV_DEVICE_NAME,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + },
> +};
> +
> +static int __init ssam_debug_init(void)
> +{
> + int status;
> +
> + ssam_cdev_device = platform_device_alloc(SSAM_CDEV_DEVICE_NAME,
> + PLATFORM_DEVID_NONE);
> + if (!ssam_cdev_device)
> + return -ENOMEM;
> +
> + status = platform_device_add(ssam_cdev_device);
> + if (status)
> + goto err_device;
> +
> + status = platform_driver_register(&ssam_cdev_driver);
> + if (status)
> + goto err_driver;
> +
> + return 0;
> +
> +err_driver:
> + platform_device_del(ssam_cdev_device);
> +err_device:
> + platform_device_put(ssam_cdev_device);
> + return status;
> +}
> +module_init(ssam_debug_init);
> +
> +static void __exit ssam_debug_exit(void)
> +{
> + platform_driver_unregister(&ssam_cdev_driver);
> + platform_device_unregister(ssam_cdev_device);
> +}
> +module_exit(ssam_debug_exit);
> +
> +MODULE_AUTHOR("Maximilian Luz <luzmaximilian@gmail.com>");
> +MODULE_DESCRIPTION("User-space interface for Surface System Aggregator Module");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/surface_aggregator/cdev.h b/include/uapi/linux/surface_aggregator/cdev.h
> new file mode 100644
> index 000000000000..1a8bc0249f8e
> --- /dev/null
> +++ b/include/uapi/linux/surface_aggregator/cdev.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +/*
> + * Surface System Aggregator Module (SSAM) user-space EC interface.
> + *
> + * Definitions, structs, and IOCTLs for the /dev/surface/aggregator misc
> + * device. This device provides direct user-space access to the SSAM EC.
> + * Intended for debugging and development.
> + *
> + * Copyright (C) 2020 Maximilian Luz <luzmaximilian@gmail.com>
> + */
> +
> +#ifndef _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H
> +#define _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct ssam_cdev_request - Controller request IOCTL argument.
> + * @target_category: Target category of the SAM request.
> + * @target_id: Target ID of the SAM request.
> + * @command_id: Command ID of the SAM request.
> + * @instance_id: Instance ID of the SAM request.
> + * @flags: SAM Request flags.
> + * @status: Request status (output).
> + * @payload: Request payload (input data).
> + * @payload.data: Pointer to request payload data.
> + * @payload.length: Length of request payload data (in bytes).
> + * @response: Request response (output data).
> + * @response.data: Pointer to response buffer.
> + * @response.length: On input: Capacity of response buffer (in bytes).
> + * On output: Length of request response (number of bytes
> + * in the buffer that are actually used).
> + */
> +struct ssam_cdev_request {
> + __u8 target_category;
> + __u8 target_id;
> + __u8 command_id;
> + __u8 instance_id;
> + __u16 flags;
> + __s16 status;
> +
> + struct {
> + __u64 data;
> + __u16 length;
> + __u8 __pad[6];
> + } payload;
> +
> + struct {
> + __u64 data;
> + __u16 length;
> + __u8 __pad[6];
> + } response;
> +} __attribute__((__packed__));
> +
> +#define SSAM_CDEV_REQUEST _IOWR(0xA5, 1, struct ssam_cdev_request)
> +
> +#endif /* _UAPI_LINUX_SURFACE_AGGREGATOR_CDEV_H */
> --
> 2.29.2
>