require EXPORT_SYMBOL_GPL symbols for symbol_get

require EXPORT_SYMBOL_GPL symbols for symbol_get

[Christoph Hellwig]

Hi all,

this series changes symbol_get to only work on EXPORT_SYMBOL_GPL
as nvidia is abusing the lack of this check to bypass restrictions
on importing symbols from proprietary modules.

[PATCH 1/5] ARM/pxa: use EXPORT_SYMBOL_GPL for sharpsl_battery_kick

[Christoph Hellwig]

sharpsl_battery_kick is only used via symbol_get, which was only ever
intended for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
for it so that symbol_get can enforce only being used on
EXPORT_SYMBOL_GPL symbols.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm/mach-pxa/sharpsl_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-pxa/sharpsl_pm.c b/arch/arm/mach-pxa/sharpsl_pm.c
index d29bdcd5270e0f..08eec58632c988 100644
--- a/arch/arm/mach-pxa/sharpsl_pm.c
+++ b/arch/arm/mach-pxa/sharpsl_pm.c
@@ -216,7 +216,7 @@ void sharpsl_battery_kick(void)
 {
 	schedule_delayed_work(&sharpsl_bat, msecs_to_jiffies(125));
 }
-EXPORT_SYMBOL(sharpsl_battery_kick);
+EXPORT_SYMBOL_GPL(sharpsl_battery_kick);
 
 
 static void sharpsl_battery_thread(struct work_struct *private_)
-- 
2.39.2

Re: [PATCH 1/5] ARM/pxa: use EXPORT_SYMBOL_GPL for sharpsl_battery_kick

[Arnd Bergmann]

On Mon, Jul 31, 2023, at 10:38, Christoph Hellwig wrote:
> sharpsl_battery_kick is only used via symbol_get, which was only ever
> intended for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
> for it so that symbol_get can enforce only being used on
> EXPORT_SYMBOL_GPL symbols.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

The reasoning makes sense, and the patch looks good, so feel
free to take this through your tree.

Acked-by: Arnd Bergmann <arnd@arndb.de>

Or let me know if you want a better fix. Since sharpsl_pm.c and
spitz.c are no longer loadable modules and just get linked together
these days, I think the variant below would be simpler (this could
be cleanup up further, endlessly, of course):

--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -518,17 +518,6 @@ static struct gpiod_lookup_table spitz_ads7846_gpio_table = {
        },
 };
 
-static void spitz_bl_kick_battery(void)
-{
-       void (*kick_batt)(void);
-
-       kick_batt = symbol_get(sharpsl_battery_kick);
-       if (kick_batt) {
-               kick_batt();
-               symbol_put(sharpsl_battery_kick);
-       }
-}
-
 static struct gpiod_lookup_table spitz_lcdcon_gpio_table = {
        .dev_id = "spi2.1",
        .table = {
@@ -556,7 +545,7 @@ static struct corgi_lcd_platform_data spitz_lcdcon_info = {
        .max_intensity          = 0x2f,
        .default_intensity      = 0x1f,
        .limit_mask             = 0x0b,
-       .kick_battery           = spitz_bl_kick_battery,
+       .kick_battery           = sharpsl_battery_kick,
 };
 
 static struct spi_board_info spitz_spi_devices[] = {

Re: [PATCH 1/5] ARM/pxa: use EXPORT_SYMBOL_GPL for sharpsl_battery_kick

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 06:12:22PM +0200, Arnd Bergmann wrote:
> Or let me know if you want a better fix. Since sharpsl_pm.c and
> spitz.c are no longer loadable modules and just get linked together
> these days, I think the variant below would be simpler (this could
> be cleanup up further, endlessly, of course):

That actually looks way nicer, thanks!

If you give me a singoff I'll add it to the next version.

[PATCH 2/5] net: enetc: use EXPORT_SYMBOL_GPL for enetc_phc_index

[Christoph Hellwig]

enetc_phc_index is only used via symbol_get, which was only ever
intended for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
for it so that symbol_get can enforce only being used on
EXPORT_SYMBOL_GPL symbols.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/net/ethernet/freescale/enetc/enetc_ptp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc_ptp.c b/drivers/net/ethernet/freescale/enetc/enetc_ptp.c
index 17c097cef7d45f..5243fc03105890 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc_ptp.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc_ptp.c
@@ -8,7 +8,7 @@
 #include "enetc.h"
 
 int enetc_phc_index = -1;
-EXPORT_SYMBOL(enetc_phc_index);
+EXPORT_SYMBOL_GPL(enetc_phc_index);
 
 static struct ptp_clock_info enetc_ptp_caps = {
 	.owner		= THIS_MODULE,
-- 
2.39.2

Re: [PATCH 2/5] net: enetc: use EXPORT_SYMBOL_GPL for enetc_phc_index

[Jakub Kicinski]

On Mon, 31 Jul 2023 10:38:03 +0200 Christoph Hellwig wrote:
> enetc_phc_index is only used via symbol_get, which was only ever
> intended for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
> for it so that symbol_get can enforce only being used on
> EXPORT_SYMBOL_GPL symbols.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

[PATCH 3/5] rtc: ds1685: use EXPORT_SYMBOL_GPL for ds1685_rtc_poweroff

[Christoph Hellwig]

ds1685_rtc_poweroff is only used externally via symbol_get, which was
only ever intended for very internal symbols like this one.  Use
EXPORT_SYMBOL_GPL for it so that symbol_get can enforce only being used
on EXPORT_SYMBOL_GPL symbols.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/rtc/rtc-ds1685.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
index 0f707be0eb87fa..04dbf35cf3b706 100644
--- a/drivers/rtc/rtc-ds1685.c
+++ b/drivers/rtc/rtc-ds1685.c
@@ -1432,7 +1432,7 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
 		unreachable();
 	}
 }
-EXPORT_SYMBOL(ds1685_rtc_poweroff);
+EXPORT_SYMBOL_GPL(ds1685_rtc_poweroff);
 /* ----------------------------------------------------------------------- */
 
 
-- 
2.39.2

Re: [PATCH 3/5] rtc: ds1685: use EXPORT_SYMBOL_GPL for ds1685_rtc_poweroff

[Joshua Kinard]

On 7/31/2023 04:38, Christoph Hellwig wrote:
> ds1685_rtc_poweroff is only used externally via symbol_get, which was
> only ever intended for very internal symbols like this one.  Use
> EXPORT_SYMBOL_GPL for it so that symbol_get can enforce only being used
> on EXPORT_SYMBOL_GPL symbols.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/rtc/rtc-ds1685.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/rtc/rtc-ds1685.c b/drivers/rtc/rtc-ds1685.c
> index 0f707be0eb87fa..04dbf35cf3b706 100644
> --- a/drivers/rtc/rtc-ds1685.c
> +++ b/drivers/rtc/rtc-ds1685.c
> @@ -1432,7 +1432,7 @@ ds1685_rtc_poweroff(struct platform_device *pdev)
>   		unreachable();
>   	}
>   }
> -EXPORT_SYMBOL(ds1685_rtc_poweroff);
> +EXPORT_SYMBOL_GPL(ds1685_rtc_poweroff);
>   /* ----------------------------------------------------------------------- */
>   
>   

Acked-by: Joshua Kinard <kumba@gentoo.org>

[PATCH 4/5] mmc: use EXPORT_SYMBOL_GPL for mmc_detect_change

[Christoph Hellwig]

mmc_detect_change is used via symbol_get, which was only ever intended
for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
for it so that symbol_get can enforce only being used on
EXPORT_SYMBOL_GPL symbols.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 3d3e0ca5261481..8ffd78fae1e7b9 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1433,7 +1433,7 @@ void mmc_detect_change(struct mmc_host *host, unsigned long delay)
 {
 	_mmc_detect_change(host, delay, true);
 }
-EXPORT_SYMBOL(mmc_detect_change);
+EXPORT_SYMBOL_GPL(mmc_detect_change);
 
 void mmc_init_erase(struct mmc_card *card)
 {
-- 
2.39.2

Re: [PATCH 4/5] mmc: use EXPORT_SYMBOL_GPL for mmc_detect_change

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 10:38:05AM +0200, Christoph Hellwig wrote:
> mmc_detect_change is used via symbol_get, which was only ever intended
> for very internal symbols like this one.  Use EXPORT_SYMBOL_GPL
> for it so that symbol_get can enforce only being used on
> EXPORT_SYMBOL_GPL symbols.

Btw, I really wonder if this should actually be used through symbol_get.
It seems like the MIPS/alchemy boards should simply require MMC to be
built in and not modular, or the IRQ handlers should move into a driver.

That would be a much less mechanical change, but this use really looks
a bit odd.  And makes me wonder if we should only allow symbol_get
on symbols specifically marked to supported it, but that would be
another incremental step.

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

[Christoph Hellwig]

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 <hch@lst.de>
---
 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

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

[Greg Kroah-Hartman]

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 <hch@lst.de>

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

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

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

[Simon Horman]

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

...

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

[Luis Chamberlain]

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

Re: require EXPORT_SYMBOL_GPL symbols for symbol_get

[Greg Kroah-Hartman]

On Mon, Jul 31, 2023 at 10:38:01AM +0200, Christoph Hellwig wrote:
> Hi all,
> 
> this series changes symbol_get to only work on EXPORT_SYMBOL_GPL
> as nvidia is abusing the lack of this check to bypass restrictions
> on importing symbols from proprietary modules.

For the whole series:

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