2023-07-31 09:44:59

by Christoph Hellwig

[permalink] [raw]
Subject: [PATCH 5/5] modules: only allow symbol_get of EXPORT_SYMBOL_GPL modules

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 propriertary
modules into an allegedly GPL licensed module and then rexporting them.

Given that symbol_get was only ever inteded for tightly cooperating
modules using very internal symbols it is logical to restrict it to
being used on EXPORY_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 <[email protected]>
---
kernel/module/internal.h | 1 +
kernel/module/main.c | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c8b7b4dcf7820d..add687c2abde8b 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -93,6 +93,7 @@ struct find_symbol_arg {
/* Input */
const char *name;
bool gplok;
+ bool gplonly;
bool warn;

/* Output */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 59b1d067e52890..85d3f00ca65758 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -281,6 +281,8 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,

if (!fsa->gplok && syms->license == GPL_ONLY)
return false;
+ if (fsa->gplonly && syms->license != GPL_ONLY)
+ return false;

sym = bsearch(fsa->name, syms->start, syms->stop - syms->start,
sizeof(struct kernel_symbol), cmp_name);
@@ -776,8 +778,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
void __symbol_put(const char *symbol)
{
struct find_symbol_arg fsa = {
- .name = symbol,
- .gplok = true,
+ .name = symbol,
+ .gplok = true,
+ .gplonly = true,
};

preempt_disable();
@@ -1289,14 +1292,18 @@ static void free_module(struct module *mod)
void *__symbol_get(const char *symbol)
{
struct find_symbol_arg fsa = {
- .name = symbol,
- .gplok = true,
- .warn = true,
+ .name = symbol,
+ .gplok = true,
+ .gplonly = true,
+ .warn = true,
};

preempt_disable();
if (!find_symbol(&fsa) || strong_try_module_get(fsa.owner)) {
preempt_enable();
+ if (fsa.gplonly)
+ pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",
+ symbol);
return NULL;
}
preempt_enable();
--
2.39.2



2023-07-31 12:18:15

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 5/5] modules: only allow symbol_get of EXPORT_SYMBOL_GPL modules

On Mon, Jul 31, 2023 at 10:38:06AM +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 propriertary
> modules into an allegedly GPL licensed module and then rexporting them.
>
> Given that symbol_get was only ever inteded for tightly cooperating
> modules using very internal symbols it is logical to restrict it to
> being used on EXPORY_SYMBOL_GPL and prevent nvidia from costly DMCA

"EXPORT" :)

> 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 <[email protected]>

Thanks for fixing this hole up, it's much needed.

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

2023-07-31 19:05:29

by Simon Horman

[permalink] [raw]
Subject: Re: [PATCH 5/5] modules: only allow symbol_get of EXPORT_SYMBOL_GPL modules

On Mon, Jul 31, 2023 at 10:38:06AM +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 propriertary
> modules into an allegedly GPL licensed module and then rexporting them.
>
> Given that symbol_get was only ever inteded for tightly cooperating

nit: inteded -> intended

...

2023-07-31 21:10:04

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH 5/5] modules: only allow symbol_get of EXPORT_SYMBOL_GPL modules

On Mon, Jul 31, 2023 at 10:38:06AM +0200, Christoph Hellwig wrote:
> ---
> kernel/module/internal.h | 1 +
> kernel/module/main.c | 17 ++++++++++++-----
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index c8b7b4dcf7820d..add687c2abde8b 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -93,6 +93,7 @@ struct find_symbol_arg {
> /* Input */
> const char *name;
> bool gplok;
> + bool gplonly;

We'd want to add here a reason or something like that to allow the
caller to know why we failed if we want to provide feedback.

> bool warn;
>
> /* Output */
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 59b1d067e52890..85d3f00ca65758 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -281,6 +281,8 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
>
> if (!fsa->gplok && syms->license == GPL_ONLY)
> return false;
> + if (fsa->gplonly && syms->license != GPL_ONLY)

And set it here to something other than perhaps a default of NOT_FOUND.

> + return false;
>
> sym = bsearch(fsa->name, syms->start, syms->stop - syms->start,
> sizeof(struct kernel_symbol), cmp_name);
> @@ -776,8 +778,9 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
> @@ -1289,14 +1292,18 @@ static void free_module(struct module *mod)
> void *__symbol_get(const char *symbol)
> {
> struct find_symbol_arg fsa = {
> - .name = symbol,
> - .gplok = true,
> - .warn = true,
> + .name = symbol,
> + .gplok = true,
> + .gplonly = true,
> + .warn = true,
> };
>
> preempt_disable();
> if (!find_symbol(&fsa) || strong_try_module_get(fsa.owner)) {
> preempt_enable();
> + if (fsa.gplonly)
> + pr_warn("failing symbol_get of non-GPLONLY symbol %s.\n",

Because here fsa.gplonly is always true here so the above warn will
print even if a symbol is just not found.

Luis