On Fri, Aug 08, 2014 at 05:06:41PM -0700, Kees Cook wrote:
> On Fri, Aug 8, 2014 at 10:11 AM, Dan Streetman <ddstr...@ieee.org> wrote:
> > On Fri, Aug 8, 2014 at 3:53 AM, Kees Cook <keesc...@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 <keesc...@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 <sjenn...@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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
