diff mbox series

[5/5] modules: only allow symbol_get of EXPORT_SYMBOL_GPL modules

Message ID 20230801173544.1929519-6-hch@lst.de (mailing list archive)
State New, archived
Headers show
Series [1/5] ARM: pxa: remove use of symbol_get() | expand

Commit Message

Christoph Hellwig Aug. 1, 2023, 5:35 p.m. UTC
It has recently come to my attention that nvidia is circumventing the
protection added in 262e6ae7081d ("modules: inherit
TAINT_PROPRIETARY_MODULE") by importing exports from their proprietary
modules into an allegedly GPL licensed module and then rexporting them.

Given that symbol_get was only ever intended for tightly cooperating
modules using very internal symbols it is logical to restrict it to
being used on EXPORT_SYMBOL_GPL and prevent nvidia from costly DMCA
Circumvention of Access Controls law suites.

All symbols except for four used through symbol_get were already exported
as EXPORT_SYMBOL_GPL, and the remaining four ones were switched over in
the preparation patches.

Fixes: 262e6ae7081d ("modules: inherit TAINT_PROPRIETARY_MODULE")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 kernel/module/main.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

David Woodhouse Oct. 18, 2023, 12:30 a.m. UTC | #1
On Tue, 2023-08-01 at 19:35 +0200, Christoph Hellwig wrote:
> It has recently come to my attention that nvidia is circumventing the
> protection added in 262e6ae7081d ("modules: inherit
> TAINT_PROPRIETARY_MODULE") by importing exports from their proprietary
> modules into an allegedly GPL licensed module and then rexporting them.
> 
> Given that symbol_get was only ever intended for tightly cooperating
> modules using very internal symbols it is logical to restrict it to
> being used on EXPORT_SYMBOL_GPL and prevent nvidia from costly DMCA
> Circumvention of Access Controls law suites.

I'm all for insisting that everything be exported with
EXPORT_SYMBOL_GPL and nothing at all ever be exported with just
EXPORT_SYMBOL.

But if we're going to tolerate the core kernel still exporting some
stuff with EXPORT_SYMBOL, why isn't OK for a GPL-licensed module do to
the same? Even an *in-tree* GPL-licensed module now can't export
functionality with EXPORT_SYMBOL and have it used with symbol_get().

We're forced to *either* allow direct linking by non-GPL modules, or
allow symbol_get(), but not both?

> Fixes: 262e6ae7081d ("modules: inherit TAINT_PROPRIETARY_MODULE")

Hm, the condition we really need to fix *that* is "symbol_get() will
only import symbols from GPL-licensed modules", isn't it?

As long as that property is correctly transitive, why does the symbol
itself have to be EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL? Am I
missing another potential loophole?

I suppose there's now scope for a different type of shim which
*directly* imports an EXPORT_SYMBOL function in order to export it
again as EXPORT_SYMBOL_GPL and thus allow the GPL export to be found
with symbol_get()?

That's the *converse* of the problematic shim that was being used
before, and from a licensing point of view it seems fine... it's just
working around the unintended side-effects of this patch?
Christoph Hellwig Oct. 18, 2023, 5:31 a.m. UTC | #2
On Wed, Oct 18, 2023 at 01:30:18AM +0100, David Woodhouse wrote:
> 
> But if we're going to tolerate the core kernel still exporting some
> stuff with EXPORT_SYMBOL, why isn't OK for a GPL-licensed module do to
> the same? Even an *in-tree* GPL-licensed module now can't export
> functionality with EXPORT_SYMBOL and have it used with symbol_get().

Anything using symbol_get is by intent very deeply internal for tightly
coupled modules working together, and thus not a non-GPL export.

In fact the current series is just a stepping stone.  Once some mess
in the kvm/vfio integration is fixed up we'll require a new explicit
EXPORT_SYMBOL variant as symbol_get wasn't ever intended to be used
on totally random symbols not exported for use by symbol_get.
Luis Chamberlain Oct. 18, 2023, 6:25 p.m. UTC | #3
On Wed, Oct 18, 2023 at 07:31:46AM +0200, Christoph Hellwig wrote:
> On Wed, Oct 18, 2023 at 01:30:18AM +0100, David Woodhouse wrote:
> > 
> > But if we're going to tolerate the core kernel still exporting some
> > stuff with EXPORT_SYMBOL, why isn't OK for a GPL-licensed module do to
> > the same? Even an *in-tree* GPL-licensed module now can't export
> > functionality with EXPORT_SYMBOL and have it used with symbol_get().
> 
> Anything using symbol_get is by intent very deeply internal for tightly
> coupled modules working together, and thus not a non-GPL export.
> 
> In fact the current series is just a stepping stone.  Once some mess
> in the kvm/vfio integration is fixed up we'll require a new explicit
> EXPORT_SYMBOL variant as symbol_get wasn't ever intended to be used
> on totally random symbols not exported for use by symbol_get.

The later patches in the series also show we could resolves most
uses through Kconfig and at build time, it really begs the question
if we even need it for any real valid uses.

  Luis
diff mbox series

Patch

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 59b1d067e52890..c395af9eced114 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1295,12 +1295,20 @@  void *__symbol_get(const char *symbol)
 	};
 
 	preempt_disable();
-	if (!find_symbol(&fsa) || strong_try_module_get(fsa.owner)) {
-		preempt_enable();
-		return NULL;
+	if (!find_symbol(&fsa))
+		goto fail;
+	if (fsa.license != GPL_ONLY) {
+		pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",
+			symbol);
+		goto fail;
 	}
+	if (strong_try_module_get(fsa.owner))
+		goto fail;
 	preempt_enable();
 	return (void *)kernel_symbol_value(fsa.sym);
+fail:
+	preempt_enable();
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(__symbol_get);