inherit TAINT_PROPRIETARY_MODULE

inherit TAINT_PROPRIETARY_MODULE

[Christoph Hellwig]

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/6376CA34-BC6F-45DE-9FFD-7E32664C7569@fb.com/T/#md514322fdfa212afe9f1d3eb4e5f7eaefece36eb

Diffstat:
 include/linux/module.h |   24 ++----------------------
 kernel/module.c        |   42 +++++++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 37 deletions(-)

[PATCH 1/7] modules: mark ref_module static

[Christoph Hellwig]

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

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/module.h | 1 -
 kernel/module.c        | 6 ++----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 2e6670860d275f..f1fdbeef2153a8 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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)			\
diff --git a/kernel/module.c b/kernel/module.c
index aa183c9ac0a256..17d64dae756c80 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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)
 {
-- 
2.27.0

[PATCH 2/7] modules: mark find_symbol static

[Christoph Hellwig]

find_symbol is only used in module.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/module.h | 11 -----------
 kernel/module.c        |  3 +--
 2 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index f1fdbeef2153a8..90bdc362be3681 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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
  *
diff --git a/kernel/module.c b/kernel/module.c
index 17d64dae756c80..84da96a6d8241c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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
-- 
2.27.0

[PATCH 3/7] modules: mark each_symbol_section static

[Christoph Hellwig]

each_symbol_section is only used inside of module.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/module.h | 9 ---------
 kernel/module.c        | 3 +--
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 90bdc362be3681..b79219eed83c56 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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,
diff --git a/kernel/module.c b/kernel/module.c
index 84da96a6d8241c..feeaa9629eb179 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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 */
-- 
2.27.0

[PATCH 4/7] modules: unexport __module_text_address

[Christoph Hellwig]

__module_text_address is only used by built-in code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/module.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index feeaa9629eb179..d241866f9d4a2b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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)
-- 
2.27.0

[PATCH 5/7] modules: unexport __module_address

[Christoph Hellwig]

__module_address is only used by built-in code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/module.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index d241866f9d4a2b..54e853c7212f72 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -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?
-- 
2.27.0

[PATCH 6/7] modules: return licensing information from find_symbol

[Christoph Hellwig]

Report the GPLONLY status through a new argument.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/module.h |  2 +-
 kernel/module.c        | 16 +++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index b79219eed83c56..c9bc3412ae4465 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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_licence {
 		NOT_GPL_ONLY,
 		GPL_ONLY,
 		WILL_BE_GPL_ONLY,
diff --git a/kernel/module.c b/kernel/module.c
index 54e853c7212f72..a907bc57d343f9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -495,6 +495,7 @@ struct find_symbol_arg {
 	struct module *owner;
 	const s32 *crc;
 	const struct kernel_symbol *sym;
+	enum mod_licence licence;
 };
 
 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->licence = syms->licence;
 	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_licence *licence,
 					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 (licence)
+			*licence = fsa.licence;
 		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_licence licence;
 	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, &licence,
 			  !(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),
-- 
2.27.0

[PATCH 7/7] modules: inherit TAINT_PROPRIETARY_MODULE

[Christoph Hellwig]

If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the tain 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.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/module.h |  1 +
 kernel/module.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/linux/module.h b/include/linux/module.h
index c9bc3412ae4465..d49e033563dadf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -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. */
diff --git a/kernel/module.c b/kernel/module.c
index a907bc57d343f9..eb6ab2f4b81aaa 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -1456,6 +1456,18 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
 	if (!sym)
 		goto unlock;
 
+	if (licence == 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;
-- 
2.27.0

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

[Greg Kroah-Hartman]

On Wed, Jul 29, 2020 at 08:27:11AM +0200, Christoph Hellwig wrote:
> If a TAINT_PROPRIETARY_MODULE exports symbol, inherit the tain flag

s/tain/taint/

> 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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

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

Nice, work, thanks for doing this:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 2/7] modules: mark find_symbol static

[Jessica Yu]

+++ Christoph Hellwig [29/07/20 08:27 +0200]:
>find_symbol is only used in module.c.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>

CCing the livepatching ML, as this may or may not impact its users.

AFAIK, the out-of-tree kpatch module had used find_symbol() in the
past, I am not sure what its current status is. I suspect all of its
functionality has been migrated to upstream livepatch already.

>---
> include/linux/module.h | 11 -----------
> kernel/module.c        |  3 +--
> 2 files changed, 1 insertion(+), 13 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index f1fdbeef2153a8..90bdc362be3681 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -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
>  *
>diff --git a/kernel/module.c b/kernel/module.c
>index 17d64dae756c80..84da96a6d8241c 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -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
>-- 
>2.27.0
>

Re: [PATCH 2/7] modules: mark find_symbol static

[Greg Kroah-Hartman]

On Wed, Jul 29, 2020 at 06:13:18PM +0200, Jessica Yu wrote:
> +++ Christoph Hellwig [29/07/20 08:27 +0200]:
> > find_symbol is only used in module.c.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> CCing the livepatching ML, as this may or may not impact its users.
> 
> AFAIK, the out-of-tree kpatch module had used find_symbol() in the
> past, I am not sure what its current status is. I suspect all of its
> functionality has been migrated to upstream livepatch already.

We still have symbol_get(), which is what I thought they were using.

If only we could get rid of that export one day...

thanks,

greg k-h

Re: [PATCH 6/7] modules: return licensing information from find_symbol

[Jessica Yu]

+++ Christoph Hellwig [29/07/20 08:27 +0200]:
>Report the GPLONLY status through a new argument.
>
>Signed-off-by: Christoph Hellwig <hch@lst.de>
>---
> include/linux/module.h |  2 +-
> kernel/module.c        | 16 +++++++++++-----
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/include/linux/module.h b/include/linux/module.h
>index b79219eed83c56..c9bc3412ae4465 100644
>--- a/include/linux/module.h
>+++ b/include/linux/module.h
>@@ -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_licence {
> 		NOT_GPL_ONLY,
> 		GPL_ONLY,
> 		WILL_BE_GPL_ONLY,
>diff --git a/kernel/module.c b/kernel/module.c
>index 54e853c7212f72..a907bc57d343f9 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -495,6 +495,7 @@ struct find_symbol_arg {
> 	struct module *owner;
> 	const s32 *crc;
> 	const struct kernel_symbol *sym;
>+	enum mod_licence licence;

Just a small nit. Most of module.c uses license rather than licence -
could we unify the spelling to remain consistent? Sigh, American vs.
British English.. :)

Re: [PATCH 2/7] modules: mark find_symbol static

[Christoph Hellwig]

On Wed, Jul 29, 2020 at 06:24:35PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 29, 2020 at 06:13:18PM +0200, Jessica Yu wrote:
> > +++ Christoph Hellwig [29/07/20 08:27 +0200]:
> > > find_symbol is only used in module.c.
> > > 
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > 
> > CCing the livepatching ML, as this may or may not impact its users.
> > 
> > AFAIK, the out-of-tree kpatch module had used find_symbol() in the
> > past, I am not sure what its current status is. I suspect all of its
> > functionality has been migrated to upstream livepatch already.
> 
> We still have symbol_get(), which is what I thought they were using.

And even if it didn't out of tree modules really don't matter.

Re: [PATCH 6/7] modules: return licensing information from find_symbol

[Christoph Hellwig]

On Wed, Jul 29, 2020 at 06:48:00PM +0200, Jessica Yu wrote:
> Just a small nit. Most of module.c uses license rather than licence -
> could we unify the spelling to remain consistent? Sigh, American vs.
> British English.. :)

Sure, I can fix that up.

Re: [PATCH 2/7] modules: mark find_symbol static

[Joe Lawrence]

On 7/29/20 12:24 PM, Greg Kroah-Hartman wrote:
> On Wed, Jul 29, 2020 at 06:13:18PM +0200, Jessica Yu wrote:
>> +++ Christoph Hellwig [29/07/20 08:27 +0200]:
>>> find_symbol is only used in module.c.
>>>
>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>
>> CCing the livepatching ML, as this may or may not impact its users.
>>
>> AFAIK, the out-of-tree kpatch module had used find_symbol() in the
>> past, I am not sure what its current status is. I suspect all of its
>> functionality has been migrated to upstream livepatch already.
> 
> We still have symbol_get(), which is what I thought they were using.
> 

The deprecated (though still in the repo) kpatch.ko still references 
find_symbol(), but that module is no longer officially supported by the 
project.

Jessica is correct that the functionality has been migrated upstream.

I don't see any references to symbol_get() either, so we're good on that 
front, too.

Thanks,

-- Joe