[Contribution] qubes-system76-dkms (hardware enablement for System76 laptops)

[strugee]

The problem you're addressing (if any)

A number of System76 laptops come with LED keyboards which can be controlled via the keyboard in the out-of-the-box Linux distribution (Pop!_OS). However, this functionality does not work in Qubes because it lacks the out-of-tree kernel module used to control the LEDs.

Describe the solution you'd like

I have packaged the upstream System76 DKMS module as a Qubes Builder package. See https://github.com/strugee/qubes-system76-dkms. It's a little unclear from https://www.qubes-os.org/doc/package-contributions/ but AFAICT the idea is that if accepted as an official "QubesOS-contrib" module, the Qubes developers would distribute binary packages of this. Is that correct? That would be ideal since obviously Qubes OS-distributed binary packages can be trusted by users, but my distributing binary packages is much less useful because it's harder to trust me.

Where is the value to a user, and who might that user be?

The primary benefit of this is hardware enablement on these laptops. A secondary idea that I had that this packaging enables is changing the keyboard LED color to indicate security status; for example, the keyboard could turn red when the user put a window fullscreen (preventing a qube from faking the entire desktop since the keyboard color would be trusted). Or maybe the keyboard could just match the color of the currently selected window or something; there are a bunch of options here.

Describe alternatives you've considered

N/A

Additional context

I know the developers are getting pretty close to an initial testing release of Qubes 4.1, so if there's no time to review this I totally understand. Please let me know though so I know what the status is :-)

That being said it should be pretty quick to review. Upstream's source code isn't terribly complicated or anything like that. I just read through the code to check there wasn't anything overtly suspicious in there and called it a day.

Some notes, some from https://www.qubes-os.org/doc/package-contributions/#inclusion-criteria and some generally:

• This module runs strictly in dom0 and so is never exposed to input from untrusted qubes. Obviously it does talk to more parts of the hardware (in particular IIRC from reading the code it reads/writes some values to the EC) so that's a little more attack surface but it seems minimal and unavoidable.
• The code does not follow Qubes OS coding guidelines because this is upstream's code, not mine. The RPM specfile should be up to Qubes standards though (please review thoroughly though because this is basically the first RPM specfile I've ever written!)
• I have this installed in my personal dom0 and the keyboard LED controls work as expected. I did not extensively test RPM uninstall/iinstall/upgrade/etc. behavior, but I consulted existing Qubes packaging to make sure I was getting things right so I think we should be set here. Airplane mode does not work as it's supposed to; this is documented.
• There are still some things that could be improved, for example making sure the module gets into the dom0 initramfs, as well as squashing what I believe to be cosmetic issues at install-time (strugee/qubes-system76-dkms#7). But I think it's still in reasonable enough shape to submit, and the core functionality works.
• Problem: the build is not reproducible, but I can't figure out why since qubes-builder-rpm#42 landed a while ago. I ran the generated rpms through diffoscope and the only thing that was different was timestamps. Maybe my Qubes Builder is just misconfigured?

Relevant documentation you've consulted

N/A

Related, non-duplicate issues

None

[andrewdavidwong]

Assigning to @marmarek for review or other disposition.

[marmarek]

* Problem: the build is not reproducible, but I can't figure out why since qubes-builder-rpm#42 landed a while ago. I ran the generated rpms through diffoscope and the only thing that was different was timestamps. Maybe my Qubes Builder is just misconfigured?

R4.0's dom0 (fc25) has too old rpm. But R4.1 (fc31) should be ok.

[marmarek]

Review as of https://github.com/strugee/qubes-system76-dkms/commit/ddc7503c97eff0439a3541cf56f87a6c941dcc26:

spec file and related files looks file. But please clearly separate upstream project from the packaging, otherwise it will be hard to update in the future. Two options:

1. Point at upstream repository using git submodule - this is probably simpler and also handle integrity verification, as you point at specific commit
2. Point at upstream source tarball (if exists) - this generally is considered more elegant, but also is more work (you need to handle source integrity yourself - either via signature file if provided by upstream, or a hash).

I'm fine with either.

Brief instruction for the first option

1. Remove files that are from upstream repository - preferably start with a fresh git repository and include only files that you've added.
2. Add submodule: git submodule add UPSTREAM_URL DIRECTORY_NAME
3. Modify spec file to use files from DIRECTORY_NAME instead of root dir.
4. Add get-sources and verify-sources targets to toplevel Makefile:

   
   get-sources:
   git submodule update --init

verify-sources: @true

#### Brief instruction for the second option
1. Remove files that are from upstream repository - preferably start with a fresh git repository and include only files that you've added.
2. Adjust spec file to use specific upstream tarball name, possibly also adjust build steps for the directory name included in that archive.
3. Set `NO_ARCHIVE=1` in `Makefile.builder`.
4. Add `get-sources` and `verify-sources` to toplevel Makefile that downloads and verify tarball - see for example https://github.com/QubesOS/qubes-python-u2flib-host

[strugee]

@marmarek wonderful, thanks for the review! :tada:

So, the repository is actually a clone of upstream. As such the only thing that'll be needed for updating is git merge upstream/master. If that's not the only reason to separate out the packaging from upstream's code then of course I can take one of the paths you've suggested - but I wanted to ask if that approach would work ok after all before investing time into reworking it.

[marmarek]

The issue with the current approach is, looking at the repository you can't tell which file is qubes-specific and which not. And if someone modify upstream file, you'll get merge conflicts sooner or later. In fact, it is already the case for packaging related files - if upstream modify anything in debian/ dir which is removed here. Keeping upstream sources outside of the repository and only referencing it one way or another avoids this issue. And if in any case it would be necessary to modify upstream sources, you can also apply a patch - which again, would be clearly visible that it is not part of the upstream repository (but in that case, it is more preferable to submit the patch upstream).