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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxx>");
> >  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@xxxxxxxxxx>");
> > +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@xxxxxxxxxx  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/