2021-08-04 21:43:33

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v1] driver: base: Add driver filter support

On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> Intel's Trust Domain Extensions (TDX) is a new Intel technology that
> adds support for VMs to maintain confidentiality and integrity in the
> presence of an untrusted VMM.
>
> Given the VMM is an untrusted entity and the VMM presents emulated
> hardware to the guest, a threat vector similar to Thunderclap [1] is
> present. Also, for ease of deployment, it is useful to be able to use
> the same kernel binary on host and guest, but that presents a wide
> attack surface given the range of hardware supported in typical
> builds. However, TDX guests only require a small number of drivers, a
> number small enough to audit for Thunderclap style attacks, and the
> rest can be disabled via policy. Add a mechanism to filter drivers
> based on a "protected-guest trusted" indication.

So you are trying to work around the "problem" that distro kernels
provides drivers for everything by adding additional kernel complexity?

What prevents you from using a sane, stripped down, kernel image for
these vms which would keep things sane and simpler and easier to audit
and most importantly, prove, that the image is not doing something you
don't expect it to do?

Why not just make distros that want to support this type of platform,
also provide these tiny kernel images? Why are you pushing this work on
the kernel community instead?

> An alternative solution is to disable untrusted drivers using a custom
> kernel config for TDX, but such solution will break our goal of using
> same kernel binary in guest/host or in standard OS distributions. So
> it is not considered.

Why is that a goal that you _have_ to have? Who is making that
requirement?

> Driver filter framework adds support to filter drivers at boot
> time by using platform specific allow list. This is a generic
> solution that can be reused by TDX. Driver filter framework adds a
> new hook (driver_allowed()) in driver_register() interface which
> will update the "allowed" status of the driver based on platform
> specific driver allow list or deny list. During driver bind process,
> trusted status is used to determine whether to continue or deny the
> bind process. If platform does not register any allow or deny list,
> all devices will be allowed. Platforms can use wildcard like "ALL:ALL"
> in bus_name and driver_name of filter node to allow or deny all
> bus and drivers.
>
> Per driver opt-in model is also considered as an alternative design
> choice, but central allow or deny list is chosen because it is
> easier to maintain and audit. Also, "driver self serve" might be
> abused by mistake by driver authors cutting and pasting the code.
>
> Also add an option in kernel command line and sysfs to update the
> allowed or denied drivers list. Driver filter allowed status
> can be read using following command.
>
> cat /sys/bus/$bus/drivers/$driver/allowed

You added a sysfs file without Documentation/ABI/ update as well?

{sigh}

And what's wrong with the current method of removing drivers from
devices that you do not want them to be bound to? We offer that support
for all busses now that want to do it, what driver types are you needing
to "control" here that does not take advantage of the existing
infrastructure that we currently have for this type of thing?

thanks,

greg k-h


2021-08-04 21:47:42

by Andi Kleen

[permalink] [raw]
Subject: Re: [PATCH v1] driver: base: Add driver filter support


> So you are trying to work around the "problem" that distro kernels
> provides drivers for everything by adding additional kernel complexity?
>
> What prevents you from using a sane, stripped down, kernel image for
> these vms which would keep things sane and simpler and easier to audit
> and most importantly, prove, that the image is not doing something you
> don't expect it to do?
>
> Why not just make distros that want to support this type of platform,
> also provide these tiny kernel images? Why are you pushing this work on
> the kernel community instead?


Greg, I'm surprised by your comment. Traditionally we've been tried to
support all platforms in unified binary kernels and gone to considerable
complications to do so. That has been standard Linux practice for at
least the 90ies. In some cases we went to considerable pain to support
that, for example for the 5 level page tables or for paravirt ops.

Imagine there were 10 new features or platforms and they would all ask
distributions to produce custom kernels for them, they would need to
maintain 10 different kernel packages forever for all these different
cases. It's totally reasonable that they don't want to do that.

Also even if they were willing to do custom configs it's not clear how
this could be maintained and distributed. We would either have a
standard TDX config and get everyone to agree on it (very difficult). Or
some enforcement mechanism at the Kconfig level that forces most drivers
to be disabled when TDX is on, which would be also a considerable new
mechanism and complication. In addition there are drivers which are not
covered by Kconfig today (like quite a few of the basic platform
drivers), but which we still want to filter. So to implement a full
build time mechanism would likely need more changes than this relatively
straight forward run time mechanism based on the driver model.

And of course there other use cases for a run time filter mechanism. For
example let's say you want filtering for Thunderbolt security. In this
case it has to be done at runtime because it's not practical to have a
kernel that is only built for Thunderbolt drivers, but doesn't support
anything else that is on the SOC. The only sane way to handle such a
case is to make a runtime distinction.

> And what's wrong with the current method of removing drivers from
> devices that you do not want them to be bound to? We offer that support
> for all busses now that want to do it, what driver types are you needing
> to "control" here that does not take advantage of the existing
> infrastructure that we currently have for this type of thing?

I'm not sure what mechanism you're referring to here, but in general
don't want the drivers to initialize at all because they might get
exploited in any code that they execute.The intention is to disable all
drivers except for a small allow list, because it's not practical to
harden all 25M lines of Linux code.

-Andi

Subject: Re: [PATCH v1] driver: base: Add driver filter support



On 8/4/21 12:50 PM, Andi Kleen wrote:
>
>> And what's wrong with the current method of removing drivers from
>> devices that you do not want them to be bound to?  We offer that support
>> for all busses now that want to do it, what driver types are you needing
>> to "control" here that does not take advantage of the existing
>> infrastructure that we currently have for this type of thing?
>
> I'm not sure what mechanism you're referring to here, but in general don't want the drivers to
> initialize at all because they might get exploited in any code that they execute.The intention is to
> disable all drivers except for a small allow list, because it's not practical to harden all 25M
> lines of Linux code.

Yes, we are not trying to remove the drivers via sysfs. If driver
filter is enabled, "allowed" sysfs file is used to view the driver
filter status (allowed/denied). And a write to that file changes
the allowed/denied status of the driver. It has nothing to do
with bind/unbind operations.

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2021-08-05 01:56:28

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH v1] driver: base: Add driver filter support

On Wed, Aug 4, 2021 at 12:29 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Wed, Aug 04, 2021 at 10:43:22AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Intel's Trust Domain Extensions (TDX) is a new Intel technology that
> > adds support for VMs to maintain confidentiality and integrity in the
> > presence of an untrusted VMM.
> >
> > Given the VMM is an untrusted entity and the VMM presents emulated
> > hardware to the guest, a threat vector similar to Thunderclap [1] is
> > present. Also, for ease of deployment, it is useful to be able to use
> > the same kernel binary on host and guest, but that presents a wide
> > attack surface given the range of hardware supported in typical
> > builds. However, TDX guests only require a small number of drivers, a
> > number small enough to audit for Thunderclap style attacks, and the
> > rest can be disabled via policy. Add a mechanism to filter drivers
> > based on a "protected-guest trusted" indication.
>
> So you are trying to work around the "problem" that distro kernels
> provides drivers for everything by adding additional kernel complexity?
>
> What prevents you from using a sane, stripped down, kernel image for
> these vms which would keep things sane and simpler and easier to audit
> and most importantly, prove, that the image is not doing something you
> don't expect it to do?
>
> Why not just make distros that want to support this type of platform,
> also provide these tiny kernel images? Why are you pushing this work on
> the kernel community instead?

In fact, these questions are where I started when first encountering
this proposal. Andi has addressed the single kernel image constraint,
but I want to pick up on this "pushing work to the kernel community"
contention. The small list of vetted drivers that a TDX guest needs
will be built-in and maintained in the kernel by the protected guest
developer community, so no "pushing work" there. However, given that
any driver disable mechanism needs to touch the driver core I
advocated to go ahead and make this a general purpose capability to
pick up where this [1] conversation left off. I.e. a general facility
for the corner cases that modprobe and kernel config policy can not
reach. Corner cases like VMM attacking the VM, or broken hardware with
a built-in driver that can't be unbound after the fact.

[1]: https://lore.kernel.org/lkml/[email protected]/

>
> > An alternative solution is to disable untrusted drivers using a custom
> > kernel config for TDX, but such solution will break our goal of using
> > same kernel binary in guest/host or in standard OS distributions. So
> > it is not considered.
>
> Why is that a goal that you _have_ to have? Who is making that
> requirement?
>
> > Driver filter framework adds support to filter drivers at boot
> > time by using platform specific allow list. This is a generic
> > solution that can be reused by TDX. Driver filter framework adds a
> > new hook (driver_allowed()) in driver_register() interface which
> > will update the "allowed" status of the driver based on platform
> > specific driver allow list or deny list. During driver bind process,
> > trusted status is used to determine whether to continue or deny the
> > bind process. If platform does not register any allow or deny list,
> > all devices will be allowed. Platforms can use wildcard like "ALL:ALL"
> > in bus_name and driver_name of filter node to allow or deny all
> > bus and drivers.
> >
> > Per driver opt-in model is also considered as an alternative design
> > choice, but central allow or deny list is chosen because it is
> > easier to maintain and audit. Also, "driver self serve" might be
> > abused by mistake by driver authors cutting and pasting the code.
> >
> > Also add an option in kernel command line and sysfs to update the
> > allowed or denied drivers list. Driver filter allowed status
> > can be read using following command.
> >
> > cat /sys/bus/$bus/drivers/$driver/allowed
>
> You added a sysfs file without Documentation/ABI/ update as well?
>
> {sigh}

Argh, my fault, that one slipped through in the internal review.

2021-08-05 08:59:20

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v1] driver: base: Add driver filter support

On Wed, Aug 04, 2021 at 01:09:07PM -0700, Kuppuswamy, Sathyanarayanan wrote:
>
>
> On 8/4/21 12:50 PM, Andi Kleen wrote:
> >
> > > And what's wrong with the current method of removing drivers from
> > > devices that you do not want them to be bound to?? We offer that support
> > > for all busses now that want to do it, what driver types are you needing
> > > to "control" here that does not take advantage of the existing
> > > infrastructure that we currently have for this type of thing?
> >
> > I'm not sure what mechanism you're referring to here, but in general
> > don't want the drivers to initialize at all because they might get
> > exploited in any code that they execute.The intention is to disable all
> > drivers except for a small allow list, because it's not practical to
> > harden all 25M lines of Linux code.
>
> Yes, we are not trying to remove the drivers via sysfs. If driver
> filter is enabled, "allowed" sysfs file is used to view the driver
> filter status (allowed/denied). And a write to that file changes
> the allowed/denied status of the driver. It has nothing to do
> with bind/unbind operations.

Again, we have this already today, with full sysfs control in userspace.
Why add yet-another-way to do this? What is lacking in the existing
functionality that needs to be expanded on?

greg k-h

2021-08-05 08:59:47

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v1] driver: base: Add driver filter support

On Wed, Aug 04, 2021 at 12:50:24PM -0700, Andi Kleen wrote:
>
> > So you are trying to work around the "problem" that distro kernels
> > provides drivers for everything by adding additional kernel complexity?
> >
> > What prevents you from using a sane, stripped down, kernel image for
> > these vms which would keep things sane and simpler and easier to audit
> > and most importantly, prove, that the image is not doing something you
> > don't expect it to do?
> >
> > Why not just make distros that want to support this type of platform,
> > also provide these tiny kernel images? Why are you pushing this work on
> > the kernel community instead?
>
>
> Greg, I'm surprised by your comment. Traditionally we've been tried to
> support all platforms in unified binary kernels and gone to considerable
> complications to do so. That has been standard Linux practice for at least
> the 90ies. In some cases we went to considerable pain to support that, for
> example for the 5 level page tables or for paravirt ops.
>
> Imagine there were 10 new features or platforms and they would all ask
> distributions to produce custom kernels for them, they would need to
> maintain 10 different kernel packages forever for all these different cases.
> It's totally reasonable that they don't want to do that.

Ok, but that's not what it sounded like you wanted to have here.

It looked like you wanted something like a "stripped down kernel with
only a specific number of drivers allowed to work", which for a virtual
system, would seem to imply a uniform kernel image/configuration.

> Also even if they were willing to do custom configs it's not clear how this
> could be maintained and distributed. We would either have a standard TDX
> config and get everyone to agree on it (very difficult).

Why not try that? Can't hurt and no need to change anything in the
kernel at all. 'make tdx_defconfig' should be trivial enough for you
all to maintain a single config file template, right?

> Or some enforcement
> mechanism at the Kconfig level that forces most drivers to be disabled when
> TDX is on, which would be also a considerable new mechanism and
> complication. In addition there are drivers which are not covered by Kconfig
> today (like quite a few of the basic platform drivers), but which we still
> want to filter. So to implement a full build time mechanism would likely
> need more changes than this relatively straight forward run time mechanism
> based on the driver model.
>
> And of course there other use cases for a run time filter mechanism. For
> example let's say you want filtering for Thunderbolt security. In this case
> it has to be done at runtime because it's not practical to have a kernel
> that is only built for Thunderbolt drivers, but doesn't support anything
> else that is on the SOC. The only sane way to handle such a case is to make
> a runtime distinction.

We already have filtering for thunderbolt driver security in the kernel,
why do you want to add more?

> > And what's wrong with the current method of removing drivers from
> > devices that you do not want them to be bound to? We offer that support
> > for all busses now that want to do it, what driver types are you needing
> > to "control" here that does not take advantage of the existing
> > infrastructure that we currently have for this type of thing?
>
> I'm not sure what mechanism you're referring to here, but in general don't
> want the drivers to initialize at all because they might get exploited in
> any code that they execute.

That is exactly the mechanism we have today in the kernel for all busses
if they wish to take advantage of it. We have had this for all USB
drivers for well over a decade now, this is not a new feature. Please
use that instead.

> The intention is to disable all drivers except
> for a small allow list, because it's not practical to harden all 25M lines
> of Linux code.

Great, do that in userspace using the functionality we have today
please.

thanks,

greg k-h