[PATCH] mm/zpool: use prefixed module loading

[PATCH] mm/zpool: use prefixed module loading

[Kees Cook]

To avoid potential format string expansion via module parameters,
do not use the zpool type directly in request_module() without a
format string. Additionally, to avoid arbitrary modules being loaded
via zpool API (e.g. via the zswap_zpool_type module parameter) add a
"zpool-" prefix to the requested module, as well as module aliases for
the existing zpool types (zbud and zsmalloc).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 mm/zbud.c     | 1 +
 mm/zpool.c    | 2 +-
 mm/zsmalloc.c | 1 +
 3 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/zbud.c b/mm/zbud.c
index a05790b1915e..aa74f7addab1 100644
--- a/mm/zbud.c
+++ b/mm/zbud.c
@@ -619,3 +619,4 @@ module_exit(exit_zbud);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
 MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
+MODULE_ALIAS("zpool-zbud");
diff --git a/mm/zpool.c b/mm/zpool.c
index e40612a1df00..739cdf0d183a 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
 	driver = zpool_get_driver(type);
 
 	if (!driver) {
-		request_module(type);
+		request_module("zpool-%s", type);
 		driver = zpool_get_driver(type);
 	}
 
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 4e2fc83cb394..36af729eb3f6 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1199,3 +1199,4 @@ module_exit(zs_exit);
 
 MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
+MODULE_ALIAS("zpool-zsmalloc");
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Seth Jennings]

On Fri, Aug 08, 2014 at 12:53:16AM -0700, Kees Cook wrote:
> To avoid potential format string expansion via module parameters,
> do not use the zpool type directly in request_module() without a
> format string. Additionally, to avoid arbitrary modules being loaded
> via zpool API (e.g. via the zswap_zpool_type module parameter) add a
> "zpool-" prefix to the requested module, as well as module aliases for
> the existing zpool types (zbud and zsmalloc).

I didn't know that request_module() did string expansion.
Thanks for the fix!

Acked-by: Seth Jennings <sjennings@variantweb.net>

> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  mm/zbud.c     | 1 +
>  mm/zpool.c    | 2 +-
>  mm/zsmalloc.c | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/zbud.c b/mm/zbud.c
> index a05790b1915e..aa74f7addab1 100644
> --- a/mm/zbud.c
> +++ b/mm/zbud.c
> @@ -619,3 +619,4 @@ module_exit(exit_zbud);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
>  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
> +MODULE_ALIAS("zpool-zbud");
> diff --git a/mm/zpool.c b/mm/zpool.c
> index e40612a1df00..739cdf0d183a 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
>  	driver = zpool_get_driver(type);
>  
>  	if (!driver) {
> -		request_module(type);
> +		request_module("zpool-%s", type);
>  		driver = zpool_get_driver(type);
>  	}
>  
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 4e2fc83cb394..36af729eb3f6 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1199,3 +1199,4 @@ module_exit(zs_exit);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
> +MODULE_ALIAS("zpool-zsmalloc");
> -- 
> 1.9.1
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Dan Streetman]

On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@chromium.org> wrote:
> To avoid potential format string expansion via module parameters,
> do not use the zpool type directly in request_module() without a
> format string. Additionally, to avoid arbitrary modules being loaded
> via zpool API (e.g. via the zswap_zpool_type module parameter) add a
> "zpool-" prefix to the requested module, as well as module aliases for
> the existing zpool types (zbud and zsmalloc).
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  mm/zbud.c     | 1 +
>  mm/zpool.c    | 2 +-
>  mm/zsmalloc.c | 1 +
>  3 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/mm/zbud.c b/mm/zbud.c
> index a05790b1915e..aa74f7addab1 100644
> --- a/mm/zbud.c
> +++ b/mm/zbud.c
> @@ -619,3 +619,4 @@ module_exit(exit_zbud);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
>  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
> +MODULE_ALIAS("zpool-zbud");

If we keep this, I'd recommend putting this inside the #ifdef
CONFIG_ZPOOL section, to keep all the zpool stuff together in zbud and
zsmalloc.

> diff --git a/mm/zpool.c b/mm/zpool.c
> index e40612a1df00..739cdf0d183a 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
>         driver = zpool_get_driver(type);
>
>         if (!driver) {
> -               request_module(type);
> +               request_module("zpool-%s", type);

I agree with a change of (type) to ("%s", type), but what's the need
to prefix "zpool-"?  Anyone who has access to modify the
zswap_zpool_type parameter is already root and can just as easily load
any module they want.  Additionally, the zswap_compressor parameter
also runs through request_module() (in crypto/api.c) and could be used
to load any kernel module.

I'd prefer to leave out the "zpool-" prefix unless there is a specific
reason to include it.

>                 driver = zpool_get_driver(type);
>         }
>
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 4e2fc83cb394..36af729eb3f6 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1199,3 +1199,4 @@ module_exit(zs_exit);
>
>  MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
> +MODULE_ALIAS("zpool-zsmalloc");
> --
> 1.9.1
>
>
> --
> Kees Cook
> Chrome OS Security
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Seth Jennings]

On Fri, Aug 08, 2014 at 01:11:55PM -0400, Dan Streetman wrote:
> On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@chromium.org> wrote:
> > To avoid potential format string expansion via module parameters,
> > do not use the zpool type directly in request_module() without a
> > format string. Additionally, to avoid arbitrary modules being loaded
> > via zpool API (e.g. via the zswap_zpool_type module parameter) add a
> > "zpool-" prefix to the requested module, as well as module aliases for
> > the existing zpool types (zbud and zsmalloc).
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  mm/zbud.c     | 1 +
> >  mm/zpool.c    | 2 +-
> >  mm/zsmalloc.c | 1 +
> >  3 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/zbud.c b/mm/zbud.c
> > index a05790b1915e..aa74f7addab1 100644
> > --- a/mm/zbud.c
> > +++ b/mm/zbud.c
> > @@ -619,3 +619,4 @@ module_exit(exit_zbud);
> >  MODULE_LICENSE("GPL");
> >  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
> >  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
> > +MODULE_ALIAS("zpool-zbud");
> 
> If we keep this, I'd recommend putting this inside the #ifdef
> CONFIG_ZPOOL section, to keep all the zpool stuff together in zbud and
> zsmalloc.
> 
> > diff --git a/mm/zpool.c b/mm/zpool.c
> > index e40612a1df00..739cdf0d183a 100644
> > --- a/mm/zpool.c
> > +++ b/mm/zpool.c
> > @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
> >         driver = zpool_get_driver(type);
> >
> >         if (!driver) {
> > -               request_module(type);
> > +               request_module("zpool-%s", type);
> 
> I agree with a change of (type) to ("%s", type), but what's the need
> to prefix "zpool-"?  Anyone who has access to modify the
> zswap_zpool_type parameter is already root and can just as easily load
> any module they want.  Additionally, the zswap_compressor parameter
> also runs through request_module() (in crypto/api.c) and could be used
> to load any kernel module.
> 
> I'd prefer to leave out the "zpool-" prefix unless there is a specific
> reason to include it.

I think I agree with this.  Having the zpool- prefix makes it to where
the would-be exploit couldn't load an arbitrary module; just those with a
zpool- prefix.  But then again, the exploit would need root privileges
to do any of this stuff and if it has that, it can just directly
load module so... yeah.

Seth

> 
> >                 driver = zpool_get_driver(type);
> >         }
> >
> > diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> > index 4e2fc83cb394..36af729eb3f6 100644
> > --- a/mm/zsmalloc.c
> > +++ b/mm/zsmalloc.c
> > @@ -1199,3 +1199,4 @@ module_exit(zs_exit);
> >
> >  MODULE_LICENSE("Dual BSD/GPL");
> >  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
> > +MODULE_ALIAS("zpool-zsmalloc");
> > --
> > 1.9.1
> >
> >
> > --
> > Kees Cook
> > Chrome OS Security
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Kees Cook]

On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@chromium.org> wrote:
>> To avoid potential format string expansion via module parameters,
>> do not use the zpool type directly in request_module() without a
>> format string. Additionally, to avoid arbitrary modules being loaded
>> via zpool API (e.g. via the zswap_zpool_type module parameter) add a
>> "zpool-" prefix to the requested module, as well as module aliases for
>> the existing zpool types (zbud and zsmalloc).
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  mm/zbud.c     | 1 +
>>  mm/zpool.c    | 2 +-
>>  mm/zsmalloc.c | 1 +
>>  3 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/zbud.c b/mm/zbud.c
>> index a05790b1915e..aa74f7addab1 100644
>> --- a/mm/zbud.c
>> +++ b/mm/zbud.c
>> @@ -619,3 +619,4 @@ module_exit(exit_zbud);
>>  MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
>>  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
>> +MODULE_ALIAS("zpool-zbud");
>
> If we keep this, I'd recommend putting this inside the #ifdef
> CONFIG_ZPOOL section, to keep all the zpool stuff together in zbud and
> zsmalloc.
>
>> diff --git a/mm/zpool.c b/mm/zpool.c
>> index e40612a1df00..739cdf0d183a 100644
>> --- a/mm/zpool.c
>> +++ b/mm/zpool.c
>> @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
>>         driver = zpool_get_driver(type);
>>
>>         if (!driver) {
>> -               request_module(type);
>> +               request_module("zpool-%s", type);
>
> I agree with a change of (type) to ("%s", type), but what's the need
> to prefix "zpool-"?  Anyone who has access to modify the
> zswap_zpool_type parameter is already root and can just as easily load
> any module they want.  Additionally, the zswap_compressor parameter
> also runs through request_module() (in crypto/api.c) and could be used
> to load any kernel module.

Yeah, the "%s" should be the absolute minimum. :)

> I'd prefer to leave out the "zpool-" prefix unless there is a specific
> reason to include it.

The reason is that the CAP_SYS_MODULE capability is supposed to be
what controls the loading of arbitrary modules, and that's separate
permission than changing module parameters via sysfs
(/sys/modules/...). Which begs the question: maybe those parameters
shouldn't be writable without CAP_SYS_MODULE? Greg, any thoughts here?
kobjects don't seem to carry any capabilities checks.

This is certainly much less serious than letting a non-root user load
an arbitrary module, but it would be great if we could have a clear
path to making sure that arbitrary module loading isn't the default
case here (given this new ability). In the past (netdev module
loading), a CVE was assigned for a CAP_NET_ADMIN privilege being able
to load arbitrary modules, so I don't see this as much different.

Ugh, yes, I didn't see the call to crypto_has_comp. Other users of
this routine use const char arrays, so there wasn't any danger here.
This would be the first user of the crypto API to expose this via a
userspace-controlled arbitrary string.

Herbert, what do you think here? I'm concerned we're going to get into
a situation like we had to deal with for netdev:

http://git.kernel.org/linus/8909c9ad8ff03611c9c96c9a92656213e4bb495b

I think we need to fix zswap now before it gets too far, and likely
adjust the crypto API to use a module prefix as well. Perhaps we need
a "crypto-" prefix?

-Kees

>
>>                 driver = zpool_get_driver(type);
>>         }
>>
>> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>> index 4e2fc83cb394..36af729eb3f6 100644
>> --- a/mm/zsmalloc.c
>> +++ b/mm/zsmalloc.c
>> @@ -1199,3 +1199,4 @@ module_exit(zs_exit);
>>
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
>> +MODULE_ALIAS("zpool-zsmalloc");
>> --
>> 1.9.1
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>



-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Dan Streetman]

On Fri, Aug 8, 2014 at 8:06 PM, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <ddstreet@ieee.org> wrote:
>> On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@chromium.org> wrote:
>>> To avoid potential format string expansion via module parameters,
>>> do not use the zpool type directly in request_module() without a
>>> format string. Additionally, to avoid arbitrary modules being loaded
>>> via zpool API (e.g. via the zswap_zpool_type module parameter) add a
>>> "zpool-" prefix to the requested module, as well as module aliases for
>>> the existing zpool types (zbud and zsmalloc).
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>>  mm/zbud.c     | 1 +
>>>  mm/zpool.c    | 2 +-
>>>  mm/zsmalloc.c | 1 +
>>>  3 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mm/zbud.c b/mm/zbud.c
>>> index a05790b1915e..aa74f7addab1 100644
>>> --- a/mm/zbud.c
>>> +++ b/mm/zbud.c
>>> @@ -619,3 +619,4 @@ module_exit(exit_zbud);
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
>>>  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
>>> +MODULE_ALIAS("zpool-zbud");
>>
>> If we keep this, I'd recommend putting this inside the #ifdef
>> CONFIG_ZPOOL section, to keep all the zpool stuff together in zbud and
>> zsmalloc.
>>
>>> diff --git a/mm/zpool.c b/mm/zpool.c
>>> index e40612a1df00..739cdf0d183a 100644
>>> --- a/mm/zpool.c
>>> +++ b/mm/zpool.c
>>> @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
>>>         driver = zpool_get_driver(type);
>>>
>>>         if (!driver) {
>>> -               request_module(type);
>>> +               request_module("zpool-%s", type);
>>
>> I agree with a change of (type) to ("%s", type), but what's the need
>> to prefix "zpool-"?  Anyone who has access to modify the
>> zswap_zpool_type parameter is already root and can just as easily load
>> any module they want.  Additionally, the zswap_compressor parameter
>> also runs through request_module() (in crypto/api.c) and could be used
>> to load any kernel module.
>
> Yeah, the "%s" should be the absolute minimum. :)
>
>> I'd prefer to leave out the "zpool-" prefix unless there is a specific
>> reason to include it.
>
> The reason is that the CAP_SYS_MODULE capability is supposed to be
> what controls the loading of arbitrary modules, and that's separate
> permission than changing module parameters via sysfs
> (/sys/modules/...). Which begs the question: maybe those parameters
> shouldn't be writable without CAP_SYS_MODULE? Greg, any thoughts here?
> kobjects don't seem to carry any capabilities checks.

For the current implementation in zswap, those parameters are only
settable at boot time - zswap isn't buildable (currently) as a module,
and those parameters are only processed during zswap init.

So I don't think there's currently any issue, as far as the zswap
module params, with any user being able to loading arbitrary modules.
Besides a user modifying the bootloader configuration, of course.

Even when/if zswap gets updated to be buildable as a module, passing
those parameters during zswap module load would, in itself, require
CAP_SYS_MODULE, since the params are only processed during module
init.

> This is certainly much less serious than letting a non-root user load
> an arbitrary module, but it would be great if we could have a clear
> path to making sure that arbitrary module loading isn't the default
> case here (given this new ability). In the past (netdev module
> loading), a CVE was assigned for a CAP_NET_ADMIN privilege being able
> to load arbitrary modules, so I don't see this as much different.
>
> Ugh, yes, I didn't see the call to crypto_has_comp. Other users of
> this routine use const char arrays, so there wasn't any danger here.
> This would be the first user of the crypto API to expose this via a
> userspace-controlled arbitrary string.
>
> Herbert, what do you think here? I'm concerned we're going to get into
> a situation like we had to deal with for netdev:
>
> http://git.kernel.org/linus/8909c9ad8ff03611c9c96c9a92656213e4bb495b
>
> I think we need to fix zswap now before it gets too far, and likely
> adjust the crypto API to use a module prefix as well. Perhaps we need
> a "crypto-" prefix?

Since (I think) this would only become a problem if/when zswap is
modified to process either zswap_compressor or zswap_zpool_type
outside of module init, maybe a comment would be enough clarifying
that restriction?  To just check CAP_SYS_MODULE if processing either
param outside of module init, if their value doesn't match the
default?


>
> -Kees
>
>>
>>>                 driver = zpool_get_driver(type);
>>>         }
>>>
>>> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>>> index 4e2fc83cb394..36af729eb3f6 100644
>>> --- a/mm/zsmalloc.c
>>> +++ b/mm/zsmalloc.c
>>> @@ -1199,3 +1199,4 @@ module_exit(zs_exit);
>>>
>>>  MODULE_LICENSE("Dual BSD/GPL");
>>>  MODULE_AUTHOR("Nitin Gupta <ngupta@vflare.org>");
>>> +MODULE_ALIAS("zpool-zsmalloc");
>>> --
>>> 1.9.1
>>>
>>>
>>> --
>>> Kees Cook
>>> Chrome OS Security
>>>
>>> --
>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>> the body to majordomo@kvack.org.  For more info on Linux MM,
>>> see: http://www.linux-mm.org/ .
>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
>
>
> --
> Kees Cook
> Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Herbert Xu]

On Fri, Aug 08, 2014 at 05:06:41PM -0700, Kees Cook wrote:
>
> I think we need to fix zswap now before it gets too far, and likely
> adjust the crypto API to use a module prefix as well. Perhaps we need
> a "crypto-" prefix?

Yes I think a crypto- prefix would make sense.  Most crypto
algorithms should be providing an alias already so it's mostly
just changing the aliases.

Patches are welcome :)

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Greg KH]

On Fri, Aug 08, 2014 at 05:06:41PM -0700, Kees Cook wrote:
> On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <ddstreet@ieee.org> wrote:
> > On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@chromium.org> wrote:
> >> To avoid potential format string expansion via module parameters,
> >> do not use the zpool type directly in request_module() without a
> >> format string. Additionally, to avoid arbitrary modules being loaded
> >> via zpool API (e.g. via the zswap_zpool_type module parameter) add a
> >> "zpool-" prefix to the requested module, as well as module aliases for
> >> the existing zpool types (zbud and zsmalloc).
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >>  mm/zbud.c     | 1 +
> >>  mm/zpool.c    | 2 +-
> >>  mm/zsmalloc.c | 1 +
> >>  3 files changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/mm/zbud.c b/mm/zbud.c
> >> index a05790b1915e..aa74f7addab1 100644
> >> --- a/mm/zbud.c
> >> +++ b/mm/zbud.c
> >> @@ -619,3 +619,4 @@ module_exit(exit_zbud);
> >>  MODULE_LICENSE("GPL");
> >>  MODULE_AUTHOR("Seth Jennings <sjenning@linux.vnet.ibm.com>");
> >>  MODULE_DESCRIPTION("Buddy Allocator for Compressed Pages");
> >> +MODULE_ALIAS("zpool-zbud");
> >
> > If we keep this, I'd recommend putting this inside the #ifdef
> > CONFIG_ZPOOL section, to keep all the zpool stuff together in zbud and
> > zsmalloc.
> >
> >> diff --git a/mm/zpool.c b/mm/zpool.c
> >> index e40612a1df00..739cdf0d183a 100644
> >> --- a/mm/zpool.c
> >> +++ b/mm/zpool.c
> >> @@ -150,7 +150,7 @@ struct zpool *zpool_create_pool(char *type, gfp_t gfp, struct zpool_ops *ops)
> >>         driver = zpool_get_driver(type);
> >>
> >>         if (!driver) {
> >> -               request_module(type);
> >> +               request_module("zpool-%s", type);
> >
> > I agree with a change of (type) to ("%s", type), but what's the need
> > to prefix "zpool-"?  Anyone who has access to modify the
> > zswap_zpool_type parameter is already root and can just as easily load
> > any module they want.  Additionally, the zswap_compressor parameter
> > also runs through request_module() (in crypto/api.c) and could be used
> > to load any kernel module.
> 
> Yeah, the "%s" should be the absolute minimum. :)
> 
> > I'd prefer to leave out the "zpool-" prefix unless there is a specific
> > reason to include it.
> 
> The reason is that the CAP_SYS_MODULE capability is supposed to be
> what controls the loading of arbitrary modules, and that's separate
> permission than changing module parameters via sysfs
> (/sys/modules/...). Which begs the question: maybe those parameters
> shouldn't be writable without CAP_SYS_MODULE? Greg, any thoughts here?
> kobjects don't seem to carry any capabilities checks.

Some module parameters are ment to be set by anyone, without any
capability permissions, that's why they have a file mode set on them by
the module author.  Adding a CAP_SYS_MODULE check would probably not be
a good idea.

thanks,

greg k-h

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Dan Streetman]

Ok if the crypto request_module is changed it makes more sense to
change zpool's use of request_module; it looks like there are a couple
other places in the kernel using prefixes/aliases (although it's not
universal).  I still suggest moving the MODULE_ALIAS() into
zbud/zsmalloc's #ifdef CONFIG_ZPOOL, though.


On Fri, Aug 8, 2014 at 11:08 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Aug 08, 2014 at 05:06:41PM -0700, Kees Cook wrote:
>>
>> I think we need to fix zswap now before it gets too far, and likely
>> adjust the crypto API to use a module prefix as well. Perhaps we need
>> a "crypto-" prefix?
>
> Yes I think a crypto- prefix would make sense.  Most crypto
> algorithms should be providing an alias already so it's mostly
> just changing the aliases.
>
> Patches are welcome :)
>
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm/zpool: use prefixed module loading

[Kees Cook]

On Fri, Aug 8, 2014 at 8:08 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Aug 08, 2014 at 05:06:41PM -0700, Kees Cook wrote:
>>
>> I think we need to fix zswap now before it gets too far, and likely
>> adjust the crypto API to use a module prefix as well. Perhaps we need
>> a "crypto-" prefix?
>
> Yes I think a crypto- prefix would make sense.  Most crypto
> algorithms should be providing an alias already so it's mostly
> just changing the aliases.
>
> Patches are welcome :)

Okay, I'll see the zpool patch again with the aliases moved into the
ZPOOL ifdef, and I'll start working on a crypto patch set to solve the
prefix there too.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS Security

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>