Comment 7 for bug 1991508

Revision history for this message
Rodrigo Figueiredo Zaiden (rodrigo-zaiden) wrote :

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 promoting
this to main, but it worth mentioning.
I understand that having the package in main is important from an user
perspective to enable network access for preinstalled images as this board
counts with the wifi interface as the main network interface.

There is a bug related to this driver that seems like is not reproducing
anymore, but worth looking at:
 https://bugs.launchpad.net/ubuntu/+source/linux-meta-allwinner-5.17/+bug/1994490

Security team ACK for promoting licheerv-rtl8723ds-dkms to main, but it
would be nice to check previous comment on having this driver available in
kernel packages, comments#6.