linux-kernel - Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160926092903.GC19261@mail.corp.redhat.com>
Date:   Mon, 26 Sep 2016 11:29:03 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Ioan-Adrian Ratiu <adi@...rat.com>
Cc:     jikos@...nel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Nikolai Kondrashov <spbnick@...il.com>
Subject: Re: [PATCH 2/2] hid: input: add HID_QUIRK_REUSE_AXES and fix
 dragonrise

On Sep 26 2016 or thereabouts, Ioan-Adrian Ratiu wrote:
> Commit 79346d620e9d ("HID: input: force generic axis to be mapped to their
> user space axis") made mapping generic axes to their userspace equivalents
> mandatory and some lower end gamepads which were depending on the previous
> behaviour suffered severe regressions because they were reusing axes and
> expecting hid-input to multiplex their map to the respective userspace axis
> by always searching for and using the next available axis.

Yes, I apologies for the breakage and the regression, though I must say
that for now, only one hardware maker and one device (or range of devices
from the look of it) has needed to be quirked.

> 
> Now the result is that different device axes appear on a single axis in
> userspace, which is clearly a regression in the hid-input driver because it
> needs to continue to handle this hardware as expected before the forcing
> to provide the same interface to userspace.
> 
> Since these lower-end gamepads like 0079:0006 are definitely the exception,
> create a quirk to fix them.

Given that we only have this particular vendor that is an issue, I'd
rather see the fix in hid-dr.c. The reason being that you actually don't
need to have a global quirk and this simplifies the path in hid-input.
Plus for users, they can just upgrade hid-dr without having to recompile
their kernel when hid-core is not compiled as a module. 

The cleanest solution that wouldn't require any quirk in hid-core is to
simply add an .input_mapping() callback in hid-dr.c.

The code of the callback could be something like (untested):

static int dr_input_mapping(struct hid_device *hdev, struct hid_input *hi,
		struct hid_field *field, struct hid_usage *usage,
		unsigned long **bit, int *max)
{
	switch (usage->hid) {
	/* 
	 * revert the old hid-input behavior where axes
	 * can be randomly assigned when the hid usage is
	 * reused.
	 */
	case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
	case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
		if (field->flags & HID_MAIN_ITEM_RELATIVE)
			map_rel(usage->hid & 0xf);
		else
			map_abs(usage->hid & 0xf);
		return 1;
	}
	
	return 0;
}

Hopefully, something like this should revert the old behavior for all
hid-dr touchpads.

Ideally, we need to have Dragon Rise 0x0011 tested too. Nick, would you
mind checking it if you still have this particular device?

Cheers,
Benjamin

> 
> Signed-off-by: Ioan-Adrian Ratiu <adi@...rat.com>
> ---
>  drivers/hid/hid-dr.c    |  2 ++
>  drivers/hid/hid-input.c | 16 +++++++++++-----
>  include/linux/hid.h     |  1 +
>  3 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
> index 2523f8a..27fc826 100644
> --- a/drivers/hid/hid-dr.c
> +++ b/drivers/hid/hid-dr.c
> @@ -274,6 +274,8 @@ static int dr_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  			hid_hw_stop(hdev);
>  			goto err;
>  		}
> +		/* has only 5 axes and reuses X, Y */
> +		hdev->quirks |= HID_QUIRK_REUSE_AXES;
>  		break;
>  	}
>  
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index fb9ace1..1cc6fe4 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -633,11 +633,17 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  		/* These usage IDs map directly to the usage codes. */
>  		case HID_GD_X: case HID_GD_Y: case HID_GD_Z:
>  		case HID_GD_RX: case HID_GD_RY: case HID_GD_RZ:
> -			if (field->flags & HID_MAIN_ITEM_RELATIVE)
> -				map_rel(usage->hid & 0xf);
> -			else
> -				map_abs_clear(usage->hid & 0xf);
> -			break;
> +
> +			/* if quirk is active don't force the userspace mapping,
> +			 * instead search and use the next available axis.
> +			 */
> +			if (!(device->quirks & HID_QUIRK_REUSE_AXES)) {
> +				if (field->flags & HID_MAIN_ITEM_RELATIVE)
> +					map_rel(usage->hid & 0xf);
> +				else
> +					map_abs_clear(usage->hid & 0xf);
> +				break;
> +			}
>  
>  		case HID_GD_SLIDER: case HID_GD_DIAL: case HID_GD_WHEEL:
>  			if (field->flags & HID_MAIN_ITEM_RELATIVE)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 75b66ec..0979920 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -320,6 +320,7 @@ struct hid_item {
>  #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
>  #define HID_QUIRK_ALWAYS_POLL			0x00000400
> +#define HID_QUIRK_REUSE_AXES			0x00000800
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
>  #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
>  #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	0x00040000
> -- 
> 2.10.0
>