From patchwork Sun Oct 25 09:39:10 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Herrmann X-Patchwork-Id: 7481871 Return-Path: X-Original-To: patchwork-linux-input@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 4FB70BEEA4 for ; Sun, 25 Oct 2015 09:39:17 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 2D81020796 for ; Sun, 25 Oct 2015 09:39:16 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E0D952078A for ; Sun, 25 Oct 2015 09:39:14 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751144AbbJYJjN (ORCPT ); Sun, 25 Oct 2015 05:39:13 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:35589 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750934AbbJYJjM (ORCPT ); Sun, 25 Oct 2015 05:39:12 -0400 Received: by wicll6 with SMTP id ll6so78612620wic.0; Sun, 25 Oct 2015 02:39:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=nVJRbxF61Wzf33IJfBsuaKoKG9fVDed4H1k5DPBqcow=; b=szRF/puxHsWR6qvPBKL3bQqFI7jq6FD9bg8ZPJY7udo0aMw3y7sXk1Q2HqbBsWEZCB 9+2Ig42c9J+td+O6NZrGM7GvqmCr9EB7vn57kD/bYp2Wniq0rdbk5nEfvim5G9cAxpoJ dc30Yg4Nn+gB/p9WLarGOy42EG7FWZjEWY7pghwHZicc/4Cs6a9swkcBdS4vhpwr5lNa 2NabbBqOBg9ZzV3eZrp7YeCk3LQVb12IqzwTPmHwEnHqDCgJve8x1FkDBjv9CUH3SFNO 2RFKY7lw4cLVJp2oAzAwoLPh66K5fKUxskj85le3IJiqP0FuphTBTjau6RX4U5xTjDcO IVTg== MIME-Version: 1.0 X-Received: by 10.180.37.135 with SMTP id y7mr13139998wij.89.1445765951196; Sun, 25 Oct 2015 02:39:11 -0700 (PDT) Received: by 10.194.57.130 with HTTP; Sun, 25 Oct 2015 02:39:10 -0700 (PDT) In-Reply-To: <20151024225341.GA17120@dtor-pixel> References: <1440515579-5359-1-git-send-email-benjamin.tissoires@redhat.com> <20151024225341.GA17120@dtor-pixel> Date: Sun, 25 Oct 2015 10:39:10 +0100 Message-ID: Subject: Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl From: David Herrmann To: Dmitry Torokhov Cc: Benjamin Tissoires , Peter Hutterer , "open list:HID CORE LAYER" , linux-kernel Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, T_TVD_MIME_EPI,UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi On Sun, Oct 25, 2015 at 12:53 AM, Dmitry Torokhov wrote: > Hi Benjamin, > > On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote: >> +static int uinput_abs_setup(struct uinput_device *udev, >> + struct uinput_setup __user *arg, size_t size) >> +{ >> + struct uinput_abs_setup setup = {}; >> + struct input_dev *dev; >> + >> + if (size > sizeof(setup)) >> + return -E2BIG; >> + if (udev->state == UIST_CREATED) >> + return -EINVAL; >> + if (copy_from_user(&setup, arg, size)) >> + return -EFAULT; >> + if (setup.code > ABS_MAX) >> + return -ERANGE; >> + >> + dev = udev->dev; >> + >> + input_alloc_absinfo(dev); >> + if (!dev->absinfo) >> + return -ENOMEM; >> + >> + set_bit(setup.code, dev->absbit); >> + dev->absinfo[setup.code] = setup.absinfo; >> + >> + /* >> + * We restore the state to UIST_NEW_DEVICE because the user has to call >> + * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the >> + * validity of the absbits. >> + */ > > Do we have to be this strict? It seems to me that with the new IOCTLs > you naturally want to use the new ioctl to setup the device, then adjust > various axes and bits and then validate everything. Indeed, we now force the order to be abs-setup first, then device-setup as last step. Appended is a follow-up patch to cleanup ABS handling in uinput. It is untested. Benjamin, care to give it a spin? Thanks David ---- From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Sun, 25 Oct 2015 10:34:13 +0100 Subject: [PATCH] Input: uinput - rework ABS validation Rework the uinput ABS validation to check passed absinfo data immediately, but do ABS initialization as last step in UI_DEV_CREATE. The behavior observed by user-space is not changed, as ABS initialization was never checked for errors. With this in place, the order of device-initialization and abs-configuration is no longer fixed. User-space can initialize the device and afterwards set absinfo just fine. Signed-off-by: David Herrmann --- drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 44 deletions(-) From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Sun, 25 Oct 2015 10:34:13 +0100 Subject: [PATCH] Input: uinput - rework ABS validation Rework the uinput ABS validation to check passed absinfo data immediately, but do ABS initialization as last step in UI_DEV_CREATE. The behavior observed by user-space is not changed, as ABS initialization was never checked for errors. With this in place, the order of device-initialization and abs-configuration is no longer fixed. User-space can initialize the device and afterwards set absinfo just fine. Signed-off-by: David Herrmann --- drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 44 deletions(-) diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index baac903..1d93037 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -256,13 +256,22 @@ static void uinput_destroy_device(...) static int uinput_create_device(struct uinput_device *udev) { struct input_dev *dev = udev->dev; - int error; + int error, nslot; if (udev->state != UIST_SETUP_COMPLETE) { printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME); return -EINVAL; } + if (test_bit(ABS_MT_SLOT, dev->absbit)) { + nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1; + error = input_mt_init_slots(dev, nslot, 0); + if (error) + goto fail1; + } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) { + input_set_events_per_packet(dev, 60); + } + if (udev->ff_effects_max) { error = input_ff_create(dev, udev->ff_effects_max); if (error) @@ -308,10 +317,35 @@ static int uinput_open(...) return 0; } +static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code, + const struct input_absinfo *abs) +{ + int min, max; + + min = abs->minimum; + max = abs->maximum; + + if ((min != 0 || max != 0) && max <= min) { + printk(KERN_DEBUG + "%s: invalid abs[%02x] min:%d max:%d\n", + UINPUT_NAME, code, min, max); + return -EINVAL; + } + + if (abs->flat > max - min) { + printk(KERN_DEBUG + "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n", + UINPUT_NAME, code, abs->flat, min, max); + return -EINVAL; + } + + return 0; +} + static int uinput_validate_absbits(struct input_dev *dev) { unsigned int cnt; - int nslot; + int error; if (!test_bit(EV_ABS, dev->evbit)) return 0; @@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev) */ for_each_set_bit(cnt, dev->absbit, ABS_CNT) { - int min, max; - - min = input_abs_get_min(dev, cnt); - max = input_abs_get_max(dev, cnt); - - if ((min != 0 || max != 0) && max <= min) { - printk(KERN_DEBUG - "%s: invalid abs[%02x] min:%d max:%d\n", - UINPUT_NAME, cnt, - input_abs_get_min(dev, cnt), - input_abs_get_max(dev, cnt)); + if (!dev->absinfo) return -EINVAL; - } - - if (input_abs_get_flat(dev, cnt) > - input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) { - printk(KERN_DEBUG - "%s: abs_flat #%02x out of range: %d " - "(min:%d/max:%d)\n", - UINPUT_NAME, cnt, - input_abs_get_flat(dev, cnt), - input_abs_get_min(dev, cnt), - input_abs_get_max(dev, cnt)); - return -EINVAL; - } - } - if (test_bit(ABS_MT_SLOT, dev->absbit)) { - nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1; - input_mt_init_slots(dev, nslot, 0); - } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) { - input_set_events_per_packet(dev, 60); + error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]); + if (error) + return error; } return 0; @@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev, { struct uinput_setup setup; struct input_dev *dev; - int retval; if (udev->state == UIST_CREATED) return -EINVAL; @@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev, if (!dev->name) return -ENOMEM; - retval = uinput_validate_absbits(dev); - if (retval < 0) - return retval; - udev->state = UIST_SETUP_COMPLETE; return 0; } @@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev, { struct uinput_abs_setup setup = {}; struct input_dev *dev; + int error; if (size > sizeof(setup)) return -E2BIG; @@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev, dev = udev->dev; + error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo); + if (error) + return error; + input_alloc_absinfo(dev); if (!dev->absinfo) return -ENOMEM; set_bit(setup.code, dev->absbit); dev->absinfo[setup.code] = setup.absinfo; - - /* - * We restore the state to UIST_NEW_DEVICE because the user has to call - * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the - * validity of the absbits. - */ - udev->state = UIST_NEW_DEVICE; return 0; } -- 2.6.1