NAK: [bionic][PATCH 00/10] Add drivers for RTL8821C WiFi and BT

[Seth Forshee]

On Wed, Mar 28, 2018 at 04:12:09PM +0800, Jesse Sung wrote:
> Hi Seth,
> 
> On Tue, Mar 27, 2018 at 10:10 PM, Seth Forshee
> <seth.forshee at canonical.com> wrote:
> > On Sat, Mar 24, 2018 at 12:46:57AM +0800, Wen-chien Jesse Sung wrote:
> >> BugLink: https://launchpad.net/bugs/1740231
> >> BugLink: https://launchpad.net/bugs/1742613
> >>
> >> These are based on the drivers provided by Realtek for 4.13.
> >>
> >> Since btusb will bind to this bluetooth device, it must be blacklisted
> >> in btusb to make sure that the correct driver is used. Also the table in
> >> ubuntu/rtl8821c-bt is modified so that the driver works only for this
> >> device only.
> >>
> >> Risk should be low since both drivers have no impact for systems without
> >> these devices.
> >
> > This is a driver with almost 600,000 lines of code, submitted at the
> > last minute without any explanation of why we should be adding it to our
> > kernel. Please explain why this is needed.
> 
> This device was enabled on some HP laptops. Right now we're supporting
> it with linux-oem 4.13 in Xenial. In order to keep this device working after
> upcoming linux-oem upgrade/migration, the best way is to integrate this into
> bionic kernel.

Okay, I do agree that we want to allow users to upgrade without breaking
their hw support. However this driver is ridiculously large. I have no
idea how anyone could possibly end up with a driver that has nearly
600,000 lines of code. This just doesn't belong in the distro kernel,
imo.

> > Some of my objections/questions from just a quick skim:
> >
> >  - Being built with our kernel, the driver will be signed and thus
> >    eligible to be loaded during secure boot. There's no chance that we
> >    can review it to ensure that it doesn't expose any unsafe interfaces
> >    to userspace. Have you done such a review?
> 
> For platforms w/o this device, if one can load a module to expose something,
> he can always load anything harmful directly anyway. But for platforms w/ the
> device, I'd admit that I have the same doubt as you.

We carry a set of patches for "lockdown" under secure boot. Lockdown
means that only signed modules can be loaded and a variety of
potentially insecure interfaces to userspace are disabled. If a user
wants to load an unsigned module they first have to disable secure boot
validation.

Shipping a signed module with our kernel means that we believe that
either the module doesn't expose any insecure interfaces to userspace or
else those interfaces are restricted when lockdown is enforced. I doubt
this driver has been evaluated with this in mind.

> >  - Why does this need to hook into our debian build scripts rather than
> >    being built like a normal module?
> 
> I tried to keep the modification to the driver as minimal as possible, in case
> that there's an update (a new tarball) from Realtek. But yes, I can modify
> the Makefile to make it look like a normal module if it's preferred.
> 
> >
> >  - You blacklist usb device 0bda:b00a for all architectures but only
> >    build the replacement driver for x86. Is there any chance someone
> >    might want to use the device with another architecture? If so you'll
> >    be breaking them.
> 
> Since the btusb doesn't work with this device, this may not be a problem.
> 
> Thanks,
> Jesse
> 
> >
> >  - Presumably we'll have to keep maintaining this huge pile of code for
> >    years into the future, up to and including the next LTS I expect.
> >
> > Thanks,
> > Seth