MIR licheerv-rtl8723ds-dkms

Review for Package: licheerv-rtl8723ds-dkms

[Summary]
MIR team ACK under the constraint to resolve the below listed
required TODOs and as much as possible having a look at the
recommended TODOs.
As this dkms module is interacting a lot with HAL and the radio stack, I’m not confortable enough in my code review to be bullet proof. Consequently, this does need a security review, so I'll assign ubuntu-security.
List of specific binary packages to be promoted to main: licheerv-rtl8723ds-dkms

Notes:
Required TODOs:
- There is no autopkgtests/build time test for this dkms module. There are some comments that the kernel team is going to do some testing for every release, which is OK. However, (as described in the MIR template), one requirement is to make this test plan written down (like, on the ubuntu wiki) and linked here. That way, we can ensure that every upload is tested with someone having access to the hardware as a minimal, QAed, assurance.
Recommended TODOs:
- There is no debian/watch. We also thus don’t know if the current release is packaged (we have latest commit from Git included though). Is it possible to modify debian/watch to take latest upstream main branch as it seems upstream don’t cut releases?
- It seems to me that it could be easy to fix the lintian warning (or override some of them), so that the output is cleaned when building. Mind having a look?

[Duplication]
There is no other package in main providing the same functionality.

[Dependencies]
OK:
- no other Dependencies to MIR due to this
   - SRCPKG checked with `check-mir`
- no -dev/-debug/-doc packages that need exclusion
- No dependencies in main that are only superficially tested requiring more tests now.

[Embedded sources and static linking]
OK:
- no embedded source present
- no static linking
- does not have unexpected Built-Using entries
- not a go package, no extra constraints to consider in that regard
- not a rust package, no extra constraints to consider in that regard

[Security]
OK:
- history of CVEs does not look concerning
- does not run a daemon as root (but it’s a kernel module, running as root then, see problems)
- does not use webkit1,2
- does not use lib*v8 directly
- does not open a port/socket
- does not process arbitrary web content
- does not use centralized online accounts
- does not integrate arbitrary javascript into the desktop
- does not deal with system authentication (eg, pam), etc)
- does not deal with security attestation (secure boot, tpm, signatures)
- does not deal with cryptography (en-/decryption, certificates, signing, ...)

Problems:
As part of the kernel itself, it does run as root. It’s interacting a lot with HAL and the wireless stack. It does parse many different binary protocols. This is why a security review is in order.

[Common blockers]
OK:
- does not FTBFS currently
- no new python2 dependency

Problems:
- There is no autopkgtests/build time test for this dkms module. There are some comments that the kernel team is going to do some testing for every release, which is OK. However, (as described in the MIR template), one requirement is to make this test plan written down (like, on the ubuntu wiki) and linked here. That way,...

Please find below the testplan I was implying:

$ sudo apt install licheerv-rtl8723ds-dkms
Preparing to unpack licheerv-rtl8723ds-dkms
Module licheerv-rtl8723ds-1.0 for kernel 5.17.0-1003-allwinner
Unpacking licheerv-rtl8723ds-dkms (1.0-0ubuntu1) ...
Setting up licheerv-rtl8723ds-dkms (1.0-0ubuntu1) ...
Loading new licheerv-rtl8723ds-1.0 DKMS files...
Building for 5.17.0-1003-allwinner
Building initial module for 5.17.0-1003-allwinner
Done.
8723ds.ko:
Running module version sanity check.
 - Original module
   - No original module exists within this kernel
 - Installation
   - Installing to /lib/modules/5.17.0-1003-allwinner/updates/dkms/
depmod........................

The build of the module will take quite some time (more than 1hour..).

$ sudo ip link set dev wlan0 up

And finally use the following netplan configuration to connect to your AP (to store at /etc/netplan/wifi.yaml for example):

network:
 version: 2
 renderer: networkd
 wifis:
   wlan0:
     dhcp4: yes
     dhcp6: yes
     access-points:
       "YOUR_SSID":
         password: "YOUR_PASSWORD"

$ sudo netplan apply

Finally, we should be connected to the AP:

$ ip a
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group defaul0
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
    inet 127.0.0.1/8 scope host lo
       valid_lft forever preferred_lft forever
    inet6 ::1/128 scope host
       valid_lft forever preferred_lft forever
2: usb0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc pfifo_fast state DOW0
    link/ether ae:12:05:64:8e:2c brd ff:ff:ff:ff:ff:ff
3: wlan0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc mq state UP group def0
    link/ether 34:20:03:2a:0d:0c brd ff:ff:ff:ff:ff:ff
    inet 192.168.1.28/24 metric 600 brd 192.168.1.255 scope global dynamic wlan0
       valid_lft 86393sec preferred_lft 86393sec
    inet6 fe80::3620:3ff:fe2a:d0c/64 scope link
       valid_lft forever preferred_lft forever

Status changed to 'Confirmed' because the bug affects multiple users.

> The build of the module will take quite some time (more than 1hour..).

Ouch.

Is this package a realistic way to address this need? I realize being a dkms brings a lot of benefits to developers by decoupling release cycles but I have trouble seeing users enjoying this experience.

Thanks

You're right, that's a pain. It should have been embedded in the default image so that was easier to accept...I don't have any other solution for now and people using RISC-V hardware (only developers for now) know they are very slow.

Before we start reviewing licheerv-rtl8723ds-dkms, can someone confirm that this driver hasn't been made available in the kernel packages yet?

I didn't see the reasons for this as a dkms vs integrated into the kernel in this bug -- did I overlook it?

I'm wary that this experience will be so bad that no one will want to use it.

Thanks

I reviewed licheerv-rtl8723ds-dkms 1.0-0ubuntu1.2 as checked into kinetic.
This shouldn't be considered a full audit but rather a quick gauge of
maintainability.

licheerv-rtl8723ds-dkms is the linux driver for Realtek wireless lan
RTL8723DS as used in the LicheeRV riscv board. It is packed as a dkms
package that supports build/rebuild kernel modules easily and automatically.

- CVE History:
  - No CVEs assigned
- Build-Depends?
  - debhelper, dkms (both in main)
- pre/post inst/rm scripts?
  - dh_dkms automatically adds postinst and prerm scripts.
- init scripts?
  - NA
- systemd units?
  - NA
- dbus services?
  - NA
- setuid binaries?
  - NA
- binaries in PATH?
  - NA
- sudo fragments?
  - NA
- polkit files?
  - NA
- udev rules?
  - NA
- unit tests / autopkgtests?
  - The driver itself is built when installing the package or updating the
    kernel. No autopkgtest is applied. There is a module version
    consistency check when installing the package. Loading the module, and
    connecting to an AP seems reasonable for a unit test.
- cron jobs?
  - NA
- Build logs:
  - Nothing concerning, as stated in 'unit tests / autopkgtests' section,
    the driver is built when loaded, the package build log does not have
    too much of information as expected.

- Processes spawned?
  - NA
- Memory management?
  - Coverity reported some overlapping memory issues with sprintf and
    snprintf, strings not null terminated, they were reported to upstream,
    not a blocker.
  - It does extensive use of memcpy, in general, they seem ok.
- File IO?
  - Has some usages with internal paths pre-determined.
- Logging?
  - It has wrappers around debug and logging that in general looks ok.
- Environment variable usage?
  - NA
- Use of privileged functions?
  - Use of ioctls, as a kernel driver, expected.
- Use of cryptography / random number sources etc?
  - NA
- Use of temp files?
  - NA
- Use of networking?
  - The purpose of the driver is to enable networking through wifi using
    the HAL, nothing seems concerning.
- Use of WebKit?
  - NA
- Use of PolicyKit?
  - NA

- Any significant cppcheck results?
  - NA
- Any significant Coverity results?
  - 236 reports. Some are memory corruption and seems relevant and worth
    checking, they were reported to upstream in
     https://github.com/lwfinger/rtl8723ds/issues/34
    It is ok to proceed without upstream response though, even if they are
    really a bug, they are not blockers.
- Any significant shellcheck results?
  - Some warning on legacy backticks usage, nothing really concerning.
- Any significant bandit results?
  - NA

As stated in the first review, comments#1, the main point of a security
review here is with regards to the driver interaction with HAL and wireless
stack. From a security point of view, I didn't see any really problematic
scenarios and operations.

I had some concerns over the Coverity results that were reported to
upstream, but as said before, I don't think they are a blocker. Upstream
says that the repository is a derivation from a driver owned and
distributed by Realtek. I'm not sure how much support we can expect from
what we are calling upstream and how much impact this have in prom...

A quick update over the security review:
 https://github.com/lwfinger/rtl8723ds/issues/34
was replied by upstream with some fixes at:
 https://github.com/lwfinger/rtl8723ds/commit/1a8f8b155709fb223cb9002098edc6995b1df22c

It worth taking a look on Larry's comment on the github issue regarding the mainline linux support. This dkms might be useful for a while and could be ignored in a near future. Does it change something with respect to whether we should add it to main or not?

Thanks !

Hello Rodrigo,

Could you, please, provide a link for Larry's comment that you are referring to.

Best regards

Heinrich

Hi Heinrich,

It is on the github issue,
 https://github.com/lwfinger/rtl8723ds/issues/34#issuecomment-1416595543

Thank you

The kernel patch series https://<email address hidden>/ says:

RTL8723DS may also work (we tried our best to handle rtw_chip_wcpu_11n
where needed) but has not been tested at this point.

We should check if with kernel v6.4 we can get rid of the DKMS driver.

@didrocks

In your review you asked for a test plan for the DKMS module.

Repository git://git.launchpad.net/ubuntu-manual-tests contains testcase testcases/image/1756_Install LicheeRV.

Do we need a separate document?
Where should that document be located?