Make loading of usb-hid kernel module dependent only for USB keyboard support #1145

tlaurion · 2022-04-01T20:00:15Z

@szszszsz @jans23 @kylerankin @MrChromebox : Addresses an old concern about USB HID that shouldn't be part of kernel if no USB keyboard needed: #836 (comment)

(Where USB keyboard is supported in early init code now through enable_usb if CONFIG_USB_KEYBOARD is exported per board config, while usbhid is build as a kernel module in all linux configurations as from this PR.

@DemiMarie : this should do?
@marmarek : new board addition in CI so you have a x230-hotp-maximized_usb-keyboard rom to test at each commit if wanted
@MrChromebox : I touched config/linux-librem_common.config changing usbhid.ko being built as a module, added in initrd if CONFIG_USB_KEYBOARD is added in board config
@Tonux599 : kgpe-d16-workstation_usb-keyboard is also affected here with the same reasoning (so workstation is having that usbhid.ko compiled as a module as well).

Tested: built usbhid as module (not in kernel): HOTP resealing worked after flash.
@szszszsz @kylerankin : where USB HID is needed for Nitrokey/Librem Keys under Heads?

981cb96 is added just for builds to work again (zlib 1.2.11 vanished and zlib 1.2.12 doesn't build properly as of now)

DemiMarie

Other than the trivial shell script fix below, would it be possible to check that if the module is not loaded, USB keyboards do not work but USB security dongles do? That catches both the case where the usbhid.ko module is necessary for security dongles and the case where usbhid.ko gets (wrongly) autoloaded. You might want to add usbhid.ko to a modprobe blocklist.

tlaurion · 2022-04-01T20:16:27Z

initrd/etc/functions

+	if [ $CONFIG_USB_KEYBOARD" = "y" ]; then
+		insmod /lib/modules/usbhid.ko \
+		|| die "usbhid: module load failed"


@DemiMarie there is no such thing under Heads. module loading is explicit but the scripts, where there is no blocklist.
If it is not loaded by enable_usb, per board config export (build time, or with user config override flashed back into the rom, measured and sealed by user resulting in good TOTP/HOTP, per cbfs-init code.

Ah, does Heads disable autoloading?

@DemiMarie Its litterally the other way around: there is no autoloading :)
The modules needs to be loaded in the right order to be effectively loaded here.
CONFIG_LINUX_USB_COMPANION_CONTROLLER is old USB1 (only required per KGPE-D16 boards configs)
https://github.com/osresearch/heads/blob/714dbd7ac3790322166df67fe317b170ab4e33ee/initrd/etc/functions#L92-L133

Then when usb storage is needed, the module dependencies (enable_usb) is called just right before.
https://github.com/osresearch/heads/blob/182bd6bd7517c8e322bc4712db01906b41cce58c/initrd/bin/mount-usb#L11

Thanks for the code review.

You’re welcome!

DemiMarie

Some of the kernel config changes appear unrelated.

config/linux-x230.config

tlaurion · 2022-04-02T19:33:01Z

CircleCI cache was foo bar. Kicking rebuilt will fix this once 493eb3e recreated a cache with packages being part of all cache layers (finally).

MrChromebox

I'm confused by this PR title, as the PR seems to be doing the exact opposite -- the usb-hid kernel module is being explicitly tied to, not separated from, usb keyboard support. A better title would be 'Make loading of usb-hid kernel module dependent on USB keyboard support'

The main commit here really needs to be broken down into smaller chunks:

changing usb-hid support from Y to M (and being explicit about the change)
adding the usb-hid module to the relevant linux configs
add loading of usb-hid via enable_usb
add new board x230-max-usb-kb
add CI for new board
fix: y -> "y"

Throwing all of this into a single commit makes it very difficult to parse/review/test.

DemiMarie · 2022-04-04T20:26:19Z

I'm confused by this PR title, as the PR seems to be doing the exact opposite -- the usb-hid kernel module is being explicitly tied to, not separated from, usb keyboard support. A better title would be 'Make loading of usb-hid kernel module dependent on USB keyboard support'

The main commit here really needs to be broken down into smaller chunks:

changing usb-hid support from Y to M (and being explicit about the change)

adding the usb-hid module to the relevant linux configs

add loading of usb-hid via enable_usb

add new board x230-max-usb-kb

add CI for new board

fix: y -> "y"

Throwing all of this into a single commit makes it very difficult to parse/review/test.

There also needs to be tests that USB security keys work without the usb-hid driver installed.

Testing point: - All board configs not explicitely stating export CONFIG_USB_KEYBOARD=y should not have any impact - librem_l1um, kgpe-d16_workstation-usb_keyboard, librem_mini_v2 and librem_mini will loose USB Keyboard input with this commit alone.

…NFIG_USB_KEYBOARD is defined Testing points: - None here. Board who exported "CONFIG_USB_KEYBOARD=y" have it packed under their initrd, but there is no logic loading the module yet.

Testing points: - All boards explicitely declaring CONFIG_USB_KEYBOARD=y gets USB Keyboard back under Heads - All other boards are not impacted.

- this boards is a duplicate of x230-hotp-maximized with USB Keyboard support Testing points: - x230-hotp-maximized does not accept input from USB keyboard - x230-hotp-maximized_usb-kb accepts input from USB keyboard

Testing points: - x230-hotp-maximized: USB keyboard is not working (confirmed) - x230-hotp-maximized_usb-kb: USB keyboard is working (confirmed)

tlaurion · 2022-04-05T19:08:19Z

I'm confused by this PR title, as the PR seems to be doing the exact opposite -- the usb-hid kernel module is being explicitly tied to, not separated from, usb keyboard support. A better title would be 'Make loading of usb-hid kernel module dependent on USB keyboard support'
The main commit here really needs to be broken down into smaller chunks:

changing usb-hid support from Y to M (and being explicit about the change)

adding the usb-hid module to the relevant linux configs

add loading of usb-hid via enable_usb

add new board x230-max-usb-kb

add CI for new board

~~fix: y -> "y"~~

Throwing all of this into a single commit makes it very difficult to parse/review/test.

There also needs to be tests that USB security keys work without the usb-hid driver installed.

@DemiMarie:
Was tested already in initial local PoC, also questionning Nitrokey for Nitrokey/Librem Key features requiring USB HID, while retested locally:

x230-hotp-maximized (USB keyboard doesn't work, GPG/HOTP functions works )
x230-maximized_usb-kb (USB keyboard works, GPG/HOTP functions works.)
@marmarek: That is for your custom QA setup, so you can test Ci builds directly from Heads CircleCI's builds for each commit, when/where needed.)

@MrChromebox

All done. Ping me when reviewed and/or approved.
librem_l1um librem_mini_v2 and librem_mini are affected ( and rebranded Nitrokey's @jans23? )

@kylerankin @szszszsz @jans23 @MrChromebox

Brings Use a USB security key as a TPM work-alike in the absence of a physical TPM #836 closer to being mergeable.

DemiMarie · 2022-04-05T19:56:34Z

@DemiMarie: Was tested already in initial local PoC, also questionning Nitrokey for Nitrokey/Librem Key features requiring USB HID, while retested locally:

x230-hotp-maximized (USB keyboard doesn't work, GPG/HOTP functions works )

x230-maximized_usb-kb (USB keyboard works, GPG/HOTP functions works.)

@marmarek: That is for your custom QA setup, so you can test Ci builds directly from Heads CircleCI's builds for each commit, when/where needed.)

Thank you, I suspected so, but I wanted to see it written down explicitly.

MrChromebox · 2022-04-05T21:04:47Z

There also needs to be tests that USB security keys work without the usb-hid driver installed.

Looks good here on a Librem 14 as well

tlaurion · 2022-04-06T15:16:32Z

Merging since pending question to Nitrokey is addressed under #836 (comment)

DemiMarie reviewed Apr 1, 2022

View reviewed changes

tlaurion force-pushed the seperate_usb-hid_from_usb_requirements_for_usb-keyboard_support branch 2 times, most recently from 631cbfc to 372b385 Compare April 1, 2022 20:14

DemiMarie reviewed Apr 1, 2022

View reviewed changes

config/linux-x230.config Outdated Show resolved Hide resolved

config/linux-x230.config Outdated Show resolved Hide resolved

config/linux-x230.config Outdated Show resolved Hide resolved

config/linux-x230.config Outdated Show resolved Hide resolved

tlaurion force-pushed the seperate_usb-hid_from_usb_requirements_for_usb-keyboard_support branch 4 times, most recently from 714dbd7 to 2c9a6cb Compare April 1, 2022 20:52

tlaurion mentioned this pull request Apr 1, 2022

Use a USB security key as a TPM work-alike in the absence of a physical TPM #836

Closed

MrChromebox suggested changes Apr 4, 2022

View reviewed changes

tlaurion changed the title ~~Seperate usb hid from usb requirements for usb keyboard support~~ Make loading of usb-hid kernel module dependent only for USB keyboard support Apr 5, 2022

tlaurion added 4 commits April 5, 2022 13:39

adding the usb-hid kernel module inclusion under linux module when CO…

97b124a

…NFIG_USB_KEYBOARD is defined Testing points: - None here. Board who exported "CONFIG_USB_KEYBOARD=y" have it packed under their initrd, but there is no logic loading the module yet.

functions: add loading of usb-hid via enable_usb

7b15726

Testing points: - All boards explicitely declaring CONFIG_USB_KEYBOARD=y gets USB Keyboard back under Heads - All other boards are not impacted.

add new board x230-maximized_usb-kb

6012e77

- this boards is a duplicate of x230-hotp-maximized with USB Keyboard support Testing points: - x230-hotp-maximized does not accept input from USB keyboard - x230-hotp-maximized_usb-kb accepts input from USB keyboard

tlaurion force-pushed the seperate_usb-hid_from_usb_requirements_for_usb-keyboard_support branch from 2c9a6cb to e1ca2a9 Compare April 5, 2022 18:36

CircleCI : add x230-maximized_usb-kb board

7327f35

Testing points: - x230-hotp-maximized: USB keyboard is not working (confirmed) - x230-hotp-maximized_usb-kb: USB keyboard is working (confirmed)

tlaurion force-pushed the seperate_usb-hid_from_usb_requirements_for_usb-keyboard_support branch from e1ca2a9 to 7327f35 Compare April 5, 2022 18:37

tlaurion merged commit 4bf4dcb into linuxboot:master Apr 6, 2022

tlaurion mentioned this pull request Jul 21, 2022

Talos II : Heads console output only on BMC console, not VGA Dasharo/dasharo-issues#168

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make loading of usb-hid kernel module dependent only for USB keyboard support #1145

Make loading of usb-hid kernel module dependent only for USB keyboard support #1145

tlaurion commented Apr 1, 2022 •

edited

Loading

DemiMarie left a comment

This comment was marked as outdated.

tlaurion Apr 1, 2022 •

edited

Loading

DemiMarie Apr 1, 2022

tlaurion Apr 1, 2022 •

edited

Loading

tlaurion Apr 1, 2022

DemiMarie Apr 1, 2022

This comment was marked as off-topic.

DemiMarie left a comment •

edited

Loading

tlaurion commented Apr 2, 2022 •

edited

Loading

MrChromebox left a comment

DemiMarie commented Apr 4, 2022

tlaurion commented Apr 5, 2022 •

edited

Loading

DemiMarie commented Apr 5, 2022

MrChromebox commented Apr 5, 2022

tlaurion commented Apr 6, 2022

Make loading of usb-hid kernel module dependent only for USB keyboard support #1145

Make loading of usb-hid kernel module dependent only for USB keyboard support #1145

Conversation

tlaurion commented Apr 1, 2022 • edited Loading

DemiMarie left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

tlaurion Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

DemiMarie Apr 1, 2022

Choose a reason for hiding this comment

tlaurion Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

tlaurion Apr 1, 2022

Choose a reason for hiding this comment

DemiMarie Apr 1, 2022

Choose a reason for hiding this comment

This comment was marked as off-topic.

DemiMarie left a comment • edited Loading

Choose a reason for hiding this comment

tlaurion commented Apr 2, 2022 • edited Loading

MrChromebox left a comment

Choose a reason for hiding this comment

DemiMarie commented Apr 4, 2022

tlaurion commented Apr 5, 2022 • edited Loading

DemiMarie commented Apr 5, 2022

MrChromebox commented Apr 5, 2022

tlaurion commented Apr 6, 2022

tlaurion commented Apr 1, 2022 •

edited

Loading

tlaurion Apr 1, 2022 •

edited

Loading

tlaurion Apr 1, 2022 •

edited

Loading

DemiMarie left a comment •

edited

Loading

tlaurion commented Apr 2, 2022 •

edited

Loading

tlaurion commented Apr 5, 2022 •

edited

Loading