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

lists.openwall.net		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 linux-cve-announce 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 <keescook@...omium.org>
To:	Dan Streetman <ddstreet@...e.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Seth Jennings <sjennings@...iantweb.net>,
	Minchan Kim <minchan@...nel.org>,
	Nitin Gupta <ngupta@...are.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	Linux-MM <linux-mm@...ck.org>,
	Vasiliy Kulikov <segoon@...nwall.com>
Subject: Re: [PATCH] mm/zpool: use prefixed module loading

On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <ddstreet@...e.org> wrote:
> On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keescook@...omium.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@...omium.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@...ux.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@...are.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@...ck.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@...ck.org"> email@...ck.org </a>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/