Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not allow using keyboards and mice as HID controllers #4243

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Aug 25, 2021

Enabling a keyboard or mouse as an HID controller makes it stop
working as a keyboard or mouse.

Enabling a keyboard or mouse as an HID controller makes it stop
working as a keyboard or mouse.
@Be-ing Be-ing force-pushed the hide_keyboards_and_mice branch from 6594bc8 to b4bfe76 Compare August 25, 2021 05:43
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have an exotic use case to use a second mouse for scratching. Can we preserve this without recompileing?

The root case for the linked issues seems that the mouse was enabled by default, the user setting was not persistent.

For my feeling this is the issue we should solve. Otherwise we need to Handel every single other issue of this type in our code base.

@@ -151,10 +151,8 @@ void HidController::guessDeviceCategory() {
if (hid_interface_number==-1) {
if (hid_usage_page==0x1) {
switch (hid_usage) {
case 0x2: info = tr("Generic HID Mouse"); break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seem to be unrelated.
Now it runs to the default branch which is less describtive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These devices do not appear in the GUI because they are filtered out in HidEnumerator.

(device_info.usage == kGenericDesktopMouseUsage ||
device_info.usage == kGenericDesktopKeyboardUsage)) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to the default deny list instead?
That would be the natural place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that at first but the backwards logic made it really confusing. I think this implementation is more straightforward. First there is this check for all mice and keyboards, then the denlylist is used to filter specific device models.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 25, 2021

I think we have an exotic use case to use a second mouse for scratching. Can we preserve this without recompileing?

I don't really care. That user can comment out this code and recompile. I could even comment out the code myself, push it to a branch on GitHub, and the user could download the artifact. I am much more concerned about users accidentally getting their computer (not just Mixxx!) into an unusable state, which is much more common than exotic questionably useful hacks. https://bugs.launchpad.net/mixxx/+bug/1940599 is not the first time that has happened (I do not recall where a user has asked for help for this before, but I am pretty sure it has happened).

If there was no downside to letting users treat mice and keyboards as HID DJ controllers, then yeah it'd be nice to let them do so. But the downside is that some users will get their computer into an unusable state and (rightfully IMO) blame Mixxx.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 25, 2021

The root case for the linked issues seems that the mouse was enabled by default, the user setting was not persistent.

That is a separate issue solved in #4244. The user who reported the bug was affected by both.

@uklotzde uklotzde added this to the 2.3.1 milestone Aug 25, 2021
@daschuer daschuer dismissed their stale review August 25, 2021 12:03

Ok, but not tested.

@daschuer
Copy link
Member

Is this really something we should change in 2.3?
I don't think so.
The understanding bug that the mouse is enabled unwanted should be sufficient for the stable version.
Is the issue a regression from the change to hidraw?

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 25, 2021

Is this really something we should change in 2.3?

Yes, this is a critical bug. The user can get their computer into an entirely unusable state so they have to restart it.

Is the issue a regression from the change to hidraw?

No, that is unrelated.

@Holzhaus
Copy link
Member

It's not a regression from hidraw. I have a Ducky mecha mini keyboard which shows up as a HID controller and stopped working when I enabled it in the Mixxx preferences on 2.3.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not convinced of the scattered logic. But everything is well documented and we can't rewrite it for 2.3. Works for me.

@uklotzde uklotzde added changelog This PR should be included in the changelog major bug labels Aug 25, 2021
@uklotzde
Copy link
Contributor

Any objections? Otherwise let's merge.

@Be-ing
Copy link
Contributor Author

Be-ing commented Aug 31, 2021

ping

@uklotzde
Copy link
Contributor

uklotzde commented Sep 1, 2021

I interpret the lack of responses as silent approval. LGTM

I will take care of merging into main. Already did this for testing, should not be difficult.

@uklotzde uklotzde merged commit 6cd39a5 into mixxxdj:2.3 Sep 1, 2021
@uklotzde
Copy link
Contributor

uklotzde commented Sep 1, 2021

Done. The constants have landed in hiddevice.h where they belong to instead of in hiddenylist.h.

@Holzhaus
Copy link
Member

Holzhaus commented Sep 1, 2021

Btw, I can confirm that my laptops built-in touchpad doesn't show up anymore. 👍

@foss-
Copy link
Contributor

foss- commented Sep 2, 2021

2.4-alpha-785-g705a39070b (main), Thursday, September 2, 2021 12:43:40 AM CEST
On macOS internal keyboard / trackpad is still shown. Fun fact: the T1 security chip is also shown under controllers. And while it would be very geeky run doom or mixxx on a T1 I am not sure it is a good controller to offer to users. The magic trackpad listed is the external trackpad.
Screenshot 2021-09-02 at 11 11 13

@uklotzde
Copy link
Contributor

uklotzde commented Sep 2, 2021

Please attach the enumerated HID devices from the log file.

@foss-
Copy link
Contributor

foss- commented Sep 2, 2021

Info [Controller] Scanning USB HID devices
Info [Controller] Found HID device: { 004c:0265 r0 | Usage: ff00:000b | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Duplicate HID device, excluding { 004c:0265 r0 | Usage: ff00:0014 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Found HID device: { 05ac:0278 r896 | Usage: ff00:0003 | Interface: #4 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Found HID device: { 004c:0265 r0 | Usage: ff00:0003 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Found HID device: { 05ac:0278 r0 | Usage: ff00:000f | Product: Keyboard Backlight }
Info [Controller] Excluding HID device { 05ac:0278 r896 | Usage: 0001:0002 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: 0001:0001 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: 000d:0005 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: ff00:000c | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Found HID device: { 05ac:8600 r101 | Usage: 000d:0005 | Interface: #2 | Manufacturer: Apple Inc. | Product: Apple T1 Controller }
Info [Controller] Duplicate HID device, excluding { 05ac:8600 r101 | Usage: ff00:0023 | Interface: #2 | Manufacturer: Apple Inc. | Product: Apple T1 Controller }
Info [Controller] Duplicate HID device, excluding { 05ac:8600 r101 | Usage: ff00:0016 | Interface: #2 | Manufacturer: Apple Inc. | Product: Apple T1 Controller }
Info [Controller] Duplicate HID device, excluding { 05ac:8600 r0 | Usage: 0001:0006 | Product: TouchBarUserDevice }
Info [Controller] Found HID device: { 05ac:8600 r101 | Usage: 0020:0041 | Interface: #6 | Manufacturer: Apple Inc. | Product: Apple T1 Controller }
Info [Controller] Duplicate HID device, excluding { 05ac:8600 r101 | Usage: ff12:0001 | Interface: #6 | Manufacturer: Apple Inc. | Product: Apple T1 Controller }
Info [Controller] Excluding HID device { 004c:0265 r0 | Usage: 0001:0002 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Duplicate HID device, excluding { 004c:0265 r0 | Usage: 0001:0001 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Duplicate HID device, excluding { 004c:0265 r0 | Usage: 000d:0005 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Duplicate HID device, excluding { 004c:0265 r0 | Usage: ff00:000c | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Found HID device: { 05ac:0278 r896 | Usage: ff00:000d | Interface: #3 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Found HID device: { 004c:0265 r107 | Usage: 0000:0001 | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Found HID device: { 05ac:0278 r896 | Usage: ff00:000b | Interface: #0 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Found HID device: { 004c:0265 r0 | Usage: ff00:000d | Manufacturer: Apple | Product: Magic Trackpad 2 | S/N: XXX }
Info [Controller] Excluding HID device { 05ac:0278 r896 | Usage: 0001:0006 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: 000c:0001 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: ff00:0006 | Product: Apple Internal Keyboard / Trackpad }
Info [Controller] Duplicate HID device, excluding { 05ac:0278 r896 | Usage: ff00:000f | Product: Apple Internal Keyboard / Trackpad }

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 2, 2021

Apple is likely to keep changing the product IDs of their laptop components over time. I suggest we disallow any HID device with Apple's vendor ID. Thoughts?

@foss-
Copy link
Contributor

foss- commented Sep 8, 2021

2.4-alpha-793-g169cd3e4ac (main) no longer shows Apple Internal Keyboard or Apple T1 Controller. However it still does show the external Magic Trackpad 2. Would that be expected?

Screenshot 2021-09-08 at 14 19 03

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

Taking another look at the log you posted above, that device has a different vendor ID...

@foss-
Copy link
Contributor

foss- commented Sep 8, 2021

Manufacturer: Apple | Product: Magic Trackpad 2
Manufacturer: Apple Inc. | Product: Apple T1 Controller

Right, so should that also be added to exclusion list?

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 8, 2021

I suppose so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog major bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants