lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 8 Aug 2014 17:06:41 -0700
From:	Kees Cook <>
To:	Dan Streetman <>,
	Greg KH <>,
	Herbert Xu <>
Cc:	linux-kernel <>,
	Seth Jennings <>,
	Minchan Kim <>,
	Nitin Gupta <>,
	Andrew Morton <>,
	Dan Carpenter <>,
	Linux-MM <>,
	Vasiliy Kulikov <>
Subject: Re: [PATCH] mm/zpool: use prefixed module loading

On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <> wrote:
> On Fri, Aug 8, 2014 at 3:53 AM, 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).
>> Signed-off-by: Kees Cook <>
>> ---
>>  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_AUTHOR("Seth Jennings <>");
>>  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:

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?


>>                 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_AUTHOR("Nitin Gupta <>");
>> +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  For more info on Linux MM,
>> see: .
>> Don't email: <a href=mailto:""> </a>

Kees Cook
Chrome OS Security
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

Powered by blists - more mailing lists