inherit TAINT_PROPRIETARY_MODULE v2

PATCH In review Christoph Hellwig 4 participants 18 messages Jul 30, 2020 – Aug 4, 2020 Module support

Share & tools

Christoph Hellwig

author

2020-07-30 06:10 UTC #0 lore raw

Hi Jessica,

we've had a bug in our resolution of _GPL modules since day one, that is a module can claim to be GPL licensed and use _GPL exports, while it also depends on symbols from non-GPL modules. This is used as a circumvention of the _GPL exports by using a small shim module using the _GPL exports and the other functionality. A recent example can be found here:

    https://lore.kernel.org/netdev/[email protected]/T/#md514322fdfa212afe9f1d3eb4e5f7eaefece36eb

Changes since v1:
- standardize on one spelling of "license"
- fix a commit message type

Diffstat:

 include/linux/module.h |   26 +++-----------------------
 kernel/module.c        |   46 +++++++++++++++++++++++++++++-----------------
 2 files changed, 32 insertions(+), 40 deletions(-)

Christoph Hellwig

author

2020-07-30 06:11 UTC #1 lore raw

PATCH 1/8 modules: mark ref_module static

ref_module isn't used anywhere outside of module.c.

Signed-off-by Christoph Hellwig <[email protected]>

---
 include/linux/module.h | 1 -
 kernel/module.c        | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

include/linux/module.h +0 -1

index 2e6670860d275f..f1fdbeef2153a8 100644
@@ -657,7 +657,6 @@ static inline void __module_get(struct module *module)
 #define symbol_put_addr(p) do { } while (0)
 
 #endif /* CONFIG_MODULE_UNLOAD */
-int ref_module(struct module *a, struct module *b);
 
 /* This is a #define so the string doesn't get put in every .o file */
 #define module_name(mod)			\

kernel/module.c +2 -4

index aa183c9ac0a256..17d64dae756c80 100644
@@ -869,7 +869,7 @@ static int add_module_usage(struct module *a, struct module *b)
 }
 
 /* Module a uses b: caller needs module_mutex() */
-int ref_module(struct module *a, struct module *b)
+static int ref_module(struct module *a, struct module *b)
 {
 	int err;

@@ -888,7 +888,6 @@ int ref_module(struct module *a, struct module *b)
 	}
 	return 0;
 }
-EXPORT_SYMBOL_GPL(ref_module);
 
 /* Clear the unload stuff of the module. */
 static void module_unload_free(struct module *mod)

@@ -1169,11 +1168,10 @@ static inline void module_unload_free(struct module *mod)
 {
 }
 
-int ref_module(struct module *a, struct module *b)
+static int ref_module(struct module *a, struct module *b)
 {
 	return strong_try_module_get(b);
 }
-EXPORT_SYMBOL_GPL(ref_module);
 
 static inline int module_unload_init(struct module *mod)
 {

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:11 UTC #2 lore raw

PATCH 2/8 modules: mark find_symbol static

In reply to Christoph Hellwig

find_symbol is only used in module.c.

Signed-off-by Christoph Hellwig <[email protected]>

---
 include/linux/module.h | 11 -----------
 kernel/module.c        |  3 +--
 2 files changed, 1 insertion(+), 13 deletions(-)

include/linux/module.h +0 ~~-11~~

index f1fdbeef2153a8..90bdc362be3681 100644
@@ -590,17 +590,6 @@ struct symsearch {
 	bool unused;
 };
 
-/*
- * Search for an exported symbol by name.
- *
- * Must be called with module_mutex held or preemption disabled.
- */
-const struct kernel_symbol *find_symbol(const char *name,
-					struct module **owner,
-					const s32 **crc,
-					bool gplok,
-					bool warn);
-
 /*
  * Walk the exported symbol table
  *

kernel/module.c +1 -2

index 17d64dae756c80..84da96a6d8241c 100644
@@ -585,7 +585,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
 
 /* Find an exported symbol and return it, along with, (optional) crc and
  * (optional) module which owns it.  Needs preempt disabled or module_mutex. */
-const struct kernel_symbol *find_symbol(const char *name,
+static const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,
 					const s32 **crc,
 					bool gplok,

@@ -608,7 +608,6 @@ const struct kernel_symbol *find_symbol(const char *name,
 	pr_debug("Failed to find symbol %s\n", name);
 	return NULL;
 }
-EXPORT_SYMBOL_GPL(find_symbol);
 
 /*
  * Search for module by name: must hold module_mutex (or preempt disabled

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #3 lore raw

PATCH 3/8 modules: mark each_symbol_section static

In reply to Christoph Hellwig

each_symbol_section is only used inside of module.c.

Signed-off-by Christoph Hellwig <[email protected]>

---
 include/linux/module.h | 9 ---------
 kernel/module.c        | 3 +--
 2 files changed, 1 insertion(+), 11 deletions(-)

include/linux/module.h +0 -9

index 90bdc362be3681..b79219eed83c56 100644
@@ -590,15 +590,6 @@ struct symsearch {
 	bool unused;
 };
 
-/*
- * Walk the exported symbol table
- *
- * Must be called with module_mutex held or preemption disabled.
- */
-bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
-				    struct module *owner,
-				    void *data), void *data);
-
 /* Returns 0 and fills in value, defined and namebuf, or -ERANGE if
    symnum out of range. */
 int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,

kernel/module.c +1 -2

index 84da96a6d8241c..feeaa9629eb179 100644
@@ -422,7 +422,7 @@ static bool each_symbol_in_section(const struct symsearch *arr,
 }
 
 /* Returns true as soon as fn returns true, otherwise false. */
-bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
+static bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
 				    struct module *owner,
 				    void *data),
 			 void *data)

@@ -484,7 +484,6 @@ bool each_symbol_section(bool (*fn)(const struct symsearch *arr,
 	}
 	return false;
 }
-EXPORT_SYMBOL_GPL(each_symbol_section);
 
 struct find_symbol_arg {
 	/* Input */

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #4 lore raw

PATCH 4/8 modules: unexport __module_text_address

In reply to Christoph Hellwig

__module_text_address is only used by built-in code.

Signed-off-by Christoph Hellwig <[email protected]>

---
 kernel/module.c | 1 -
 1 file changed, 1 deletion(-)

kernel/module.c +0 -1

index feeaa9629eb179..d241866f9d4a2b 100644
@@ -4508,7 +4508,6 @@ struct module *__module_text_address(unsigned long addr)
 	}
 	return mod;
 }
-EXPORT_SYMBOL_GPL(__module_text_address);
 
 /* Don't grab lock, we're oopsing. */
 void print_modules(void)

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #5 lore raw

PATCH 5/8 modules: unexport __module_address

In reply to Christoph Hellwig

__module_address is only used by built-in code.

Signed-off-by Christoph Hellwig <[email protected]>

---
 kernel/module.c | 1 -
 1 file changed, 1 deletion(-)

kernel/module.c +0 -1

index d241866f9d4a2b..54e853c7212f72 100644
@@ -4469,7 +4469,6 @@ struct module *__module_address(unsigned long addr)
 	}
 	return mod;
 }
-EXPORT_SYMBOL_GPL(__module_address);
 
 /*
  * is_module_text_address - is this address inside module code?

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #6 lore raw

PATCH 6/8 modules: rename the licence field in struct symsearch to license

In reply to Christoph Hellwig

Use the same spelling variant as the rest of the file.

Signed-off-by Christoph Hellwig <[email protected]>

---
 include/linux/module.h | 2 +-
 kernel/module.c        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

include/linux/module.h +1 -1

index b79219eed83c56..be04ba2f881daa 100644
@@ -586,7 +586,7 @@ struct symsearch {
 		NOT_GPL_ONLY,
 		GPL_ONLY,
 		WILL_BE_GPL_ONLY,
-	} licence;
+	} license;
 	bool unused;
 };

kernel/module.c +2 -2

index 54e853c7212f72..a0df783a8a6e5b 100644
@@ -504,9 +504,9 @@ static bool check_exported_symbol(const struct symsearch *syms,
 	struct find_symbol_arg *fsa = data;
 
 	if (!fsa->gplok) {
-		if (syms->licence == GPL_ONLY)
+		if (syms->license == GPL_ONLY)
 			return false;
-		if (syms->licence == WILL_BE_GPL_ONLY && fsa->warn) {
+		if (syms->license == WILL_BE_GPL_ONLY && fsa->warn) {
 			pr_warn("Symbol %s is being used by a non-GPL module, "
 				"which will not be allowed in the future\n",
 				fsa->name);

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #7 lore raw

PATCH 7/8 modules: return licensing information from find_symbol

In reply to Christoph Hellwig

Report the GPLONLY status through a new argument.

Signed-off-by Christoph Hellwig <[email protected]>

---
 include/linux/module.h |  2 +-
 kernel/module.c        | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

include/linux/module.h +1 -1

index be04ba2f881daa..30b0f5fcdb3c37 100644
@@ -582,7 +582,7 @@ struct module *find_module(const char *name);
 struct symsearch {
 	const struct kernel_symbol *start, *stop;
 	const s32 *crcs;
-	enum {
+	enum mod_license {
 		NOT_GPL_ONLY,
 		GPL_ONLY,
 		WILL_BE_GPL_ONLY,

kernel/module.c +11 -5

index a0df783a8a6e5b..afb2bfdd5134b3 100644
@@ -495,6 +495,7 @@ struct find_symbol_arg {
 	struct module *owner;
 	const s32 *crc;
 	const struct kernel_symbol *sym;
+	enum mod_license license;
 };
 
 static bool check_exported_symbol(const struct symsearch *syms,

@@ -528,6 +529,7 @@ static bool check_exported_symbol(const struct symsearch *syms,
 	fsa->owner = owner;
 	fsa->crc = symversion(syms->crcs, symnum);
 	fsa->sym = &syms->start[symnum];
+	fsa->license = syms->license;
 	return true;
 }

@@ -587,6 +589,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
 static const struct kernel_symbol *find_symbol(const char *name,
 					struct module **owner,
 					const s32 **crc,
+					enum mod_license *license,
 					bool gplok,
 					bool warn)
 {

@@ -601,6 +604,8 @@ static const struct kernel_symbol *find_symbol(const char *name,
 			*owner = fsa.owner;
 		if (crc)
 			*crc = fsa.crc;
+		if (license)
+			*license = fsa.license;
 		return fsa.sym;
 	}

@@ -1074,7 +1079,7 @@ void __symbol_put(const char *symbol)
 	struct module *owner;
 
 	preempt_disable();
-	if (!find_symbol(symbol, &owner, NULL, true, false))
+	if (!find_symbol(symbol, &owner, NULL, NULL, true, false))
 		BUG();
 	module_put(owner);
 	preempt_enable();

@@ -1352,7 +1357,7 @@ static inline int check_modstruct_version(const struct load_info *info,
 	 * locking is necessary -- use preempt_disable() to placate lockdep.
 	 */
 	preempt_disable();
-	if (!find_symbol("module_layout", NULL, &crc, true, false)) {
+	if (!find_symbol("module_layout", NULL, &crc, NULL, true, false)) {
 		preempt_enable();
 		BUG();
 	}

@@ -1436,6 +1441,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	struct module *owner;
 	const struct kernel_symbol *sym;
 	const s32 *crc;
+	enum mod_license license;
 	int err;
 
 	/*

@@ -1445,7 +1451,7 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	 */
 	sched_annotate_sleep();
 	mutex_lock(&module_mutex);
-	sym = find_symbol(name, &owner, &crc,
+	sym = find_symbol(name, &owner, &crc, &license,
 			  !(mod->taints & (1 << TAINT_PROPRIETARY_MODULE)), true);
 	if (!sym)
 		goto unlock;

@@ -2216,7 +2222,7 @@ void *__symbol_get(const char *symbol)
 	const struct kernel_symbol *sym;
 
 	preempt_disable();
-	sym = find_symbol(symbol, &owner, NULL, true, true);
+	sym = find_symbol(symbol, &owner, NULL, NULL, true, true);
 	if (sym && strong_try_module_get(owner))
 		sym = NULL;
 	preempt_enable();

@@ -2252,7 +2258,7 @@ static int verify_exported_symbols(struct module *mod)
 	for (i = 0; i < ARRAY_SIZE(arr); i++) {
 		for (s = arr[i].sym; s < arr[i].sym + arr[i].num; s++) {
 			if (find_symbol(kernel_symbol_name(s), &owner, NULL,
-					true, false)) {
+					NULL, true, false)) {
 				pr_err("%s: exports duplicate symbol %s"
 				       " (owned by %s)\n",
 				       mod->name, kernel_symbol_name(s),

Signature · 1 line

2.27.0

Christoph Hellwig

author

2020-07-30 06:10 UTC #8 lore raw

PATCH 8/8 modules: inherit TAINT_PROPRIETARY_MODULE

In reply to Christoph Hellwig

If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the taint flag for all modules importing these symbols, and don't allow loading symbols from TAINT_PROPRIETARY_MODULE modules if the module previously imported gplonly symbols. Add a anti-circumvention devices so people don't accidentally get themselves into trouble this way.

Comment from Greg:
Ah, the proven-to-be-illegal "GPL Condom" defense :)

Signed-off-by Christoph Hellwig <[email protected]>

Reviewed-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/module.h |  1 +
 kernel/module.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)

include/linux/module.h +1 -0

index 30b0f5fcdb3c37..e30ed5fa33a738 100644
@@ -389,6 +389,7 @@ struct module {
 	unsigned int num_gpl_syms;
 	const struct kernel_symbol *gpl_syms;
 	const s32 *gpl_crcs;
+	bool using_gplonly_symbols;
 
 #ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */

kernel/module.c +12 -0

index afb2bfdd5134b3..04f993863ae417 100644
@@ -1456,6 +1456,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
+	if (license == GPL_ONLY)
+		mod->using_gplonly_symbols = true;
+
+	if (owner && test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints)) {
+		if (mod->using_gplonly_symbols) {
+			sym = NULL;
+			goto getname;
+		}
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+	}
+
 	if (!check_version(info, name, mod, crc)) {
 		sym = ERR_PTR(-EINVAL);
 		goto getname;

Signature · 1 line

2.27.0

Jessica Yu

2020-07-30 14:12 UTC #9 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

+++ Christoph Hellwig [30/07/20 08:10 +0200]:

Quoted 37 lines

If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the taint flag for all modules importing these symbols, and don't allow loading symbols from TAINT_PROPRIETARY_MODULE modules if the module previously imported gplonly symbols. Add a anti-circumvention devices so people don't accidentally get themselves into trouble this way.
Comment from Greg:
Ah, the proven-to-be-illegal "GPL Condom" defense :)
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Greg Kroah-Hartman <[email protected]>
---
include/linux/module.h | 1 +
kernel/module.c | 12 ++++++++++++
2 files changed, 13 insertions(+)
include/linux/module.h +1 -0
index 30b0f5fcdb3c37..e30ed5fa33a738 100644
@@ -389,6 +389,7 @@ struct module {
 	unsigned int num_gpl_syms;
 	const struct kernel_symbol *gpl_syms;
 	const s32 *gpl_crcs;
+	bool using_gplonly_symbols;
 
 #ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */
kernel/module.c +1 -0
index afb2bfdd5134b3..04f993863ae417 100644
@@ -1456,6 +1456,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
+	if (license == GPL_ONLY)

kernel/module.c static const struct kernel_symbol *resolve_symbol(struct module *mod, · line 1469

Quoted 10 lines

kernel/module.c +10 -0

+		mod->using_gplonly_symbols = true;
+
+	if (owner && test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints)) {
+		if (mod->using_gplonly_symbols) {
+			sym = NULL;
+			goto getname;
+		}
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+	}

Sorry that I didn't think of this yesterday, but I'm wondering if we should print a warning before add_taint_module(). Maybe something along the lines of, "%s: module uses symbols from proprietary module %s, inheriting taint.", with %s being mod->name, owner->name. We can check mod->taints for TAINT_PROPRIETARY_MODULE and print the warning once.

Additionally, maybe it's a good idea to print an error before goto getname (e.g., "%s: module using GPL-only symbols uses symbols from proprietary module %s."), so one would know why the module load failed, right now this manifests itself as an unknown symbol error.

Otherwise, this patchset looks good to me and I agree with it in principle. Thanks Christoph!

Christoph Hellwig

author Reviewed-by Greg Kroah-Hartman

2020-07-30 16:30 UTC #10 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

Quoted 15 lines · Jessica Yu

Review
Quoted 8 lines
(quoted diff) +8 -0
+	if (owner && test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints)) {
+		if (mod->using_gplonly_symbols) {
+			sym = NULL;
+			goto getname;
+		}
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+	}
Sorry that I didn't think of this yesterday, but I'm wondering if we should print a warning before add_taint_module(). Maybe something along the lines of, "%s: module uses symbols from proprietary module %s, inheriting taint.", with %s being mod->name, owner->name. We can check mod->taints for TAINT_PROPRIETARY_MODULE and print the warning once.

Quoted 7 lines

Additionally, maybe it's a good idea to print an error before goto getname (e.g., "%s: module using GPL-only symbols uses symbols from proprietary module %s."), so one would know why the module load failed, right now this manifests itself as an unknown symbol error.
Otherwise, this patchset looks good to me and I agree with it in principle. Thanks Christoph!

What about this version? It also factors the code out into a new helper, and replaces the add_taint_module with a simple set_bit, as the system-wide tain must have been set before by definition:

---
From 25e928b6b691911717d30b3449e56fca3e13dba9 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Tue, 28 Jul 2020 23:33:33 +0200
Subject: modules: inherit TAINT_PROPRIETARY_MODULE

Comment from Greg:
"Ah, the proven-to-be-illegal "GPL Condom" defense :)"

Signed-off-by Christoph Hellwig <[email protected]>

Reviewed-by Greg Kroah-Hartman <[email protected]>

---
 include/linux/module.h |  1 +
 kernel/module.c        | 26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

include/linux/module.h +1 -0

index 30b0f5fcdb3c37..e30ed5fa33a738 100644
@@ -389,6 +389,7 @@ struct module {
 	unsigned int num_gpl_syms;
 	const struct kernel_symbol *gpl_syms;
 	const s32 *gpl_crcs;
+	bool using_gplonly_symbols;
 
 #ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */

kernel/module.c +26 -0

index afb2bfdd5134b3..81d5facce28c14 100644
@@ -1431,6 +1431,24 @@ static int verify_namespace_is_imported(const struct load_info *info,
 	return 0;
 }
 
+static bool inherit_taint(struct module *mod, struct module *owner)
+{
+	if (!owner || !test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints))
+		return true;
+
+	if (mod->using_gplonly_symbols) {
+		pr_info("%s: module using GPL-only symbols uses symbols from proprietary module %s.\n",
+			mod->name, owner->name);
+		return false;
+	}
+
+	if (!test_bit(TAINT_PROPRIETARY_MODULE, &mod->taints)) {
+		pr_info("%s: module uses symbols from proprietary module %s, inheriting taint.\n",
+			mod->name, owner->name);
+		set_bit(TAINT_PROPRIETARY_MODULE, &mod->taints);
+	}
+	return true;
+}
 
 /* Resolve a symbol for this module.  I.e. if we find one, record usage. */
 static const struct kernel_symbol *resolve_symbol(struct module *mod,

@@ -1456,6 +1474,14 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
+	if (license == GPL_ONLY)
+		mod->using_gplonly_symbols = true;
+
+	if (!inherit_taint(mod, owner)) {
+		sym = NULL;
+		goto getname;
+	}
+
 	if (!check_version(info, name, mod, crc)) {
 		sym = ERR_PTR(-EINVAL);
 		goto getname;

Signature · 1 line

2.27.0

Jessica Yu

2020-07-31 08:51 UTC #11 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

+++ Christoph Hellwig [30/07/20 18:29 +0200]:

Quoted 21 lines · Jessica Yu

Quoted 20 lines
Review
Quoted 8 lines
(quoted diff) +8 -0
+	if (owner && test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints)) {
+		if (mod->using_gplonly_symbols) {
+			sym = NULL;
+			goto getname;
+		}
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+	}
Sorry that I didn't think of this yesterday, but I'm wondering if we should print a warning before add_taint_module(). Maybe something along the lines of, "%s: module uses symbols from proprietary module %s, inheriting taint.", with %s being mod->name, owner->name. We can check mod->taints for TAINT_PROPRIETARY_MODULE and print the warning once.
Additionally, maybe it's a good idea to print an error before goto getname (e.g., "%s: module using GPL-only symbols uses symbols from proprietary module %s."), so one would know why the module load failed, right now this manifests itself as an unknown symbol error.

Quoted 6 lines

Quoted 2 lines
Otherwise, this patchset looks good to me and I agree with it in principle. Thanks Christoph!
What about this version? It also factors the code out into a new helper, and replaces the add_taint_module with a simple set_bit, as the system-wide tain must have been set before by definition:

Yep, this version looks much better. See below for nits.

Quoted 40 lines

---
From 25e928b6b691911717d30b3449e56fca3e13dba9 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Tue, 28 Jul 2020 23:33:33 +0200
Subject: modules: inherit TAINT_PROPRIETARY_MODULE
If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the taint flag for all modules importing these symbols, and don't allow loading symbols from TAINT_PROPRIETARY_MODULE modules if the module previously imported gplonly symbols. Add a anti-circumvention devices so people don't accidentally get themselves into trouble this way.
Comment from Greg:
"Ah, the proven-to-be-illegal "GPL Condom" defense :)"
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Greg Kroah-Hartman <[email protected]>
---
include/linux/module.h | 1 +
kernel/module.c | 26 ++++++++++++++++++++++++++
2 files changed, 27 insertions(+)
include/linux/module.h +1 -0
index 30b0f5fcdb3c37..e30ed5fa33a738 100644
@@ -389,6 +389,7 @@ struct module {
 	unsigned int num_gpl_syms;
 	const struct kernel_symbol *gpl_syms;
 	const s32 *gpl_crcs;
+	bool using_gplonly_symbols;
 
 #ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */
kernel/module.c +0 -0
index afb2bfdd5134b3..81d5facce28c14 100644
@@ -1431,6 +1431,24 @@ static int verify_namespace_is_imported(const struct load_info *info,
 	return 0;

kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1441

Quoted 10 lines

kernel/module.c +8 -0

 }
 
+static bool inherit_taint(struct module *mod, struct module *owner)
+{
+	if (!owner || !test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints))
+		return true;
+
+	if (mod->using_gplonly_symbols) {
+		pr_info("%s: module using GPL-only symbols uses symbols from proprietary module %s.\n",
+			mod->name, owner->name);

pr_err() maybe?

kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1447

Quoted 6 lines

kernel/module.c +6 -0

+		return false;
+	}
+
+	if (!test_bit(TAINT_PROPRIETARY_MODULE, &mod->taints)) {
+		pr_info("%s: module uses symbols from proprietary module %s, inheriting taint.\n",
+			mod->name, owner->name);

and pr_warn()? But otherwise this looks much better.

Thanks,

Jessica

Christoph Hellwig

author

2020-07-31 09:00 UTC #12 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

Quoted 6 lines · Jessica Yu

kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1441
Quoted 3 lines
kernel/module.c +3 -0
+	if (mod->using_gplonly_symbols) {
+		pr_info("%s: module using GPL-only symbols uses symbols from proprietary module %s.\n",
+			mod->name, owner->name);
pr_err() maybe?

Quoted 8 lines

kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1447
Quoted 6 lines
kernel/module.c +6 -0
+		return false;
+	}
+
+	if (!test_bit(TAINT_PROPRIETARY_MODULE, &mod->taints)) {
+		pr_info("%s: module uses symbols from proprietary module %s, inheriting taint.\n",
+			mod->name, owner->name);
and pr_warn()? But otherwise this looks much better.

Ok with me. Can you just fix it up, or do you want a full resend?

Jessica Yu

2020-07-31 09:02 UTC #13 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

+++ Christoph Hellwig [31/07/20 11:00 +0200]:

Quoted 16 lines · Jessica Yu

Quoted 6 lines
kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1441
Quoted 3 lines
kernel/module.c +3 -0
+	if (mod->using_gplonly_symbols) {
+		pr_info("%s: module using GPL-only symbols uses symbols from proprietary module %s.\n",
+			mod->name, owner->name);
pr_err() maybe?
Quoted 8 lines
kernel/module.c static int verify_namespace_is_imported(const struct load_info *info, · line 1447
Quoted 6 lines
kernel/module.c +6 -0
+		return false;
+	}
+
+	if (!test_bit(TAINT_PROPRIETARY_MODULE, &mod->taints)) {
+		pr_info("%s: module uses symbols from proprietary module %s, inheriting taint.\n",
+			mod->name, owner->name);
and pr_warn()? But otherwise this looks much better.

Quoted 1 line

Ok with me. Can you just fix it up, or do you want a full resend?

I can fix it up, no need to resend. Thanks!

[email protected]

Acked-by Daniel Vetter on v1 8/8

2020-08-04 18:52 UTC #14 lore raw

Re: [PATCH 8/8] modules: inherit TAINT_PROPRIETARY_MODULE

In reply to Christoph Hellwig

Quoted 11 lines · Christoph Hellwig

If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the taint flag for all modules importing these symbols, and don't allow loading symbols from TAINT_PROPRIETARY_MODULE modules if the module previously imported gplonly symbols. Add a anti-circumvention devices so people don't accidentally get themselves into trouble this way.
Comment from Greg:
Ah, the proven-to-be-illegal "GPL Condom" defense :)
Signed-off-by Christoph Hellwig <[email protected]>
Reviewed-by Greg Kroah-Hartman <[email protected]>

If this helps to get the next gpu related entertainment at least cc'ed to dri-devel, maybe even using an upstream driver, I'm all for it.

Acked-by Daniel Vetter <[email protected]>

Quoted 43 lines

---
 include/linux/module.h |  1 +
 kernel/module.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)

include/linux/module.h +1 -0

index 30b0f5fcdb3c37..e30ed5fa33a738 100644
@@ -389,6 +389,7 @@ struct module {
 	unsigned int num_gpl_syms;
 	const struct kernel_symbol *gpl_syms;
 	const s32 *gpl_crcs;
+	bool using_gplonly_symbols;
 
 #ifdef CONFIG_UNUSED_SYMBOLS
 	/* unused exported symbols. */

kernel/module.c +12 -0

index afb2bfdd5134b3..04f993863ae417 100644
@@ -1456,6 +1456,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
+	if (license == GPL_ONLY)
+		mod->using_gplonly_symbols = true;
+
+	if (owner && test_bit(TAINT_PROPRIETARY_MODULE, &owner->taints)) {
+		if (mod->using_gplonly_symbols) {
+			sym = NULL;
+			goto getname;
+		}
+		add_taint_module(mod, TAINT_PROPRIETARY_MODULE,
+				 LOCKDEP_NOW_UNRELIABLE);
+	}
+
 	if (!check_version(info, name, mod, crc)) {
 		sym = ERR_PTR(-EINVAL);
 		goto getname;

Signature · 1 line

2.27.0

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[email protected]

2020-07-31 20:11 UTC #15 lore raw

In reply to Christoph Hellwig

Quoted 5 lines · Christoph Hellwig

we've had a bug in our resolution of _GPL modules since day one, that is a module can claim to be GPL licensed and use _GPL exports, while it also depends on symbols from non-GPL modules. This is used as a circumvention of the _GPL exports by using a small shim module using the _GPL exports and the other functionality.

This looks great. You might also consider doing the reverse: if a module imports any EXPORT_SYMBOL_GPL symbols, any symbols that module in turn exports shouldn't be importable by any module that doesn't explicitly claim to be GPL-compatible. Effectively, if a module imports any EXPORT_SYMBOL_GPL symbols, all of its exported symbols would then be treated as EXPORT_SYMBOL_GPL.

This would catch the case of attempting to "wrap" EXPORT_SYMBOL_GPL symbols in the other direction, by re-exporting the same or similar functions to another module. (This would help catch mistakes, not just intentional malice.)

- Josh Triplett

Christoph Hellwig

author

2020-08-01 06:53 UTC #16 lore raw

[note: private reply now to start a flame fest with the usual suspects]

Quoted 14 lines · On Fri, Jul 31, 2020 at 01:11:46PM -0700, [email protected] wrote:

Quoted 5 lines · Christoph Hellwig
we've had a bug in our resolution of _GPL modules since day one, that is a module can claim to be GPL licensed and use _GPL exports, while it also depends on symbols from non-GPL modules. This is used as a circumvention of the _GPL exports by using a small shim module using the _GPL exports and the other functionality.
This looks great. You might also consider doing the reverse: if a module imports any EXPORT_SYMBOL_GPL symbols, any symbols that module in turn exports shouldn't be importable by any module that doesn't explicitly claim to be GPL-compatible. Effectively, if a module imports any EXPORT_SYMBOL_GPL symbols, all of its exported symbols would then be treated as EXPORT_SYMBOL_GPL.

Quoted 4 lines

This would catch the case of attempting to "wrap" EXPORT_SYMBOL_GPL symbols in the other direction, by re-exporting the same or similar functions to another module. (This would help catch mistakes, not just intentional malice.)

I'd personally 100% agree with that, but I'd rather clear it with Linus privately first. This would basically make most of the usual modular subsystems unavailable to proprietary modules as all of them use _GPL driver core exports, and I suspect he'd cave into the screaming.

Josh Triplett

2020-08-01 08:16 UTC #17 lore raw

Quoted 1 line · Christoph Hellwig

[note: private reply now to start a flame fest with the usual suspects]

[You still CCed LKML.]

Quoted 20 lines · On Fri, Jul 31, 2020 at 01:11:46PM -0700, [email protected] wrote:

Quoted 14 lines
Quoted 5 lines · Christoph Hellwig
we've had a bug in our resolution of _GPL modules since day one, that is a module can claim to be GPL licensed and use _GPL exports, while it also depends on symbols from non-GPL modules. This is used as a circumvention of the _GPL exports by using a small shim module using the _GPL exports and the other functionality.
This looks great. You might also consider doing the reverse: if a module imports any EXPORT_SYMBOL_GPL symbols, any symbols that module in turn exports shouldn't be importable by any module that doesn't explicitly claim to be GPL-compatible. Effectively, if a module imports any EXPORT_SYMBOL_GPL symbols, all of its exported symbols would then be treated as EXPORT_SYMBOL_GPL.
Quoted 4 lines
This would catch the case of attempting to "wrap" EXPORT_SYMBOL_GPL symbols in the other direction, by re-exporting the same or similar functions to another module. (This would help catch mistakes, not just intentional malice.)

Quoted 4 lines

I'd personally 100% agree with that, but I'd rather clear it with Linus privately first. This would basically make most of the usual modular subsystems unavailable to proprietary modules as all of them use _GPL driver core exports, and I suspect he'd cave into the screaming.

As a start, what about applying that logic specifically to out-of-tree modules? That would address the shim problem. The justification would be that in-tree modules have at least gone through some level of review on what they're exporting.

(Standard disclaimer: suggesting enhancements to the symbol licensing framework should not be taken as implicit endorsement of any legitimacy for non-GPL-compatible modules.)