Re: [PATCH v3] Input: uinput - Add UI_SET_PHYS_STR and UI_SET_UNIQ_STR

[Dmitry Torokhov]

On Wed, Dec 04, 2019 at 01:55:35PM -0800, Abhishek Pandit-Subedi wrote:
> The ioctl definition for UI_SET_PHYS is ambiguous because it is defined
> with size = sizeof(char*) but is expected to be given a variable length
> string. Add a deprecation notice for UI_SET_PHYS and provide
> UI_SET_PHYS_STR(len) which expects a size from the user.
> 
> Also support setting the uniq attribute of the input device. The uniq
> attribute is used as a unique identifier for the connected device.
> 
> For example, uinput devices created by BlueZ will store the address of
> the connected device as the uniq property.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> ---
> Hi input maintainers,
> 
> Here is an updated patch that refactors the ioctl handlers (properly
> allowing the size to be set from userspace). When calling the new
> ioctls, the call signature will look like this:
> 
> ```
> ioctl(fd, UI_SET_PHYS_STR(18), "00:11:22:33:44:55");
> ```
> 
> I've tested this on a Chromebook running kernel v4.19 with a sample
> program compiled for both 32-bit (i.e. gcc -m32 test.c) and 64-bit.
> 
> The final uinput device looks like this:
> 
> ```
> udevadm info -a -p /devices/virtual/input/input18
> 
> Udevadm info starts with the device specified by the devpath and then
> walks up the chain of parent devices. It prints for every device
> found, all possible attributes in the udev rules key format.
> A rule to match, can be composed by the attributes of the device
> and the attributes from one single parent device.
> 
>   looking at device '/devices/virtual/input/input18':
>     KERNEL=="input18"
>     SUBSYSTEM=="input"
>     DRIVER==""
>     ATTR{inhibited}=="0"
>     ATTR{name}=="Test"
>     ATTR{phys}=="00:00:00:33:44:55"
>     ATTR{properties}=="0"
>     ATTR{uniq}=="00:11:22:00:00:00"
> 
> ```
> 
> 
> Changes in v3:
> - Added UI_SET_PHYS_STR(len) and UI_SET_UNIQ_STR(len) and added
>   deprecation notice for UI_SET_PHYS
> 
> Changes in v2:
> - Added compat handling for UI_SET_UNIQ
> 
>  drivers/input/misc/uinput.c | 41 ++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/uinput.h |  5 +++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 84051f20b18a..27a750896c71 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -20,6 +20,7 @@
>   */
>  #include <uapi/linux/uinput.h>
>  #include <linux/poll.h>
> +#include <linux/printk.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -280,7 +281,7 @@ static int uinput_dev_flush(struct input_dev *dev, struct file *file)
>  
>  static void uinput_destroy_device(struct uinput_device *udev)
>  {
> -	const char *name, *phys;
> +	const char *name, *phys, *uniq;
>  	struct input_dev *dev = udev->dev;
>  	enum uinput_state old_state = udev->state;
>  
> @@ -289,6 +290,7 @@ static void uinput_destroy_device(struct uinput_device *udev)
>  	if (dev) {
>  		name = dev->name;
>  		phys = dev->phys;
> +		uniq = dev->uniq;
>  		if (old_state == UIST_CREATED) {
>  			uinput_flush_requests(udev);
>  			input_unregister_device(dev);
> @@ -297,6 +299,7 @@ static void uinput_destroy_device(struct uinput_device *udev)
>  		}
>  		kfree(name);
>  		kfree(phys);
> +		kfree(uniq);
>  		udev->dev = NULL;
>  	}
>  }
> @@ -840,6 +843,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	struct uinput_ff_erase  ff_erase;
>  	struct uinput_request   *req;
>  	char			*phys;
> +	char			*uniq;
>  	const char		*name;
>  	unsigned int		size;
>  
> @@ -916,6 +920,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  		goto out;
>  
>  	case UI_SET_PHYS:
> +		pr_warn_once("uinput: UI_SET_PHYS is deprecated. Use UI_SET_PHYS_STR");
> +
>  		if (udev->state == UIST_CREATED) {
>  			retval = -EINVAL;
>  			goto out;
> @@ -1023,6 +1029,39 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	case UI_ABS_SETUP & ~IOCSIZE_MASK:
>  		retval = uinput_abs_setup(udev, p, size);
>  		goto out;
> +
> +	case UI_SET_PHYS_STR(0):
> +		if (udev->state == UIST_CREATED) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		phys = strndup_user(p, size);
> +		if (IS_ERR(phys)) {
> +			retval = PTR_ERR(phys);
> +			goto out;
> +		}
> +
> +		kfree(udev->dev->phys);
> +		udev->dev->phys = phys;

Could we maybe refactor this into uinput_get_user_str(udev,
&udev->dev->phys, p, size) ?

> +		goto out;
> +
> +	case UI_SET_UNIQ_STR(0):
> +		if (udev->state == UIST_CREATED) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		uniq = strndup_user(p, size);
> +		if (IS_ERR(uniq)) {
> +			retval = PTR_ERR(uniq);
> +			goto out;
> +		}
> +
> +		kfree(udev->dev->uniq);
> +		udev->dev->uniq = uniq;
> +		goto out;
> +
>  	}
>  
>  	retval = -EINVAL;
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c9e677e3af1d..84d4fa142830 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -142,9 +142,14 @@ struct uinput_abs_setup {
>  #define UI_SET_LEDBIT		_IOW(UINPUT_IOCTL_BASE, 105, int)
>  #define UI_SET_SNDBIT		_IOW(UINPUT_IOCTL_BASE, 106, int)
>  #define UI_SET_FFBIT		_IOW(UINPUT_IOCTL_BASE, 107, int)
> +
> +/* DEPRECATED: Data size is ambiguous. Use UI_SET_PHYS_STR instead. */
>  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> +
>  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
>  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> +#define UI_SET_PHYS_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> +#define UI_SET_UNIQ_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
>  
>  #define UI_BEGIN_FF_UPLOAD	_IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload)
>  #define UI_END_FF_UPLOAD	_IOW(UINPUT_IOCTL_BASE, 201, struct uinput_ff_upload)
> -- 
> 2.24.0.393.g34dc348eaf-goog
> 

-- 
Dmitry