| 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
| ||
|
Message-ID: <5199E533.8030608@parallels.com>
Date: Mon, 20 May 2013 12:56:19 +0400
From: Stanislav Kinsbursky <skinsbursky@...allels.com>
To: Jeff Layton <jlayton@...hat.com>
CC: <akpm@...ux-foundation.org>, <lucas.demarchi@...fusion.mobi>,
<rusty@...tcorp.com.au>, <linux-kernel@...r.kernel.org>,
<oleg@...hat.com>, <bfields@...ldses.org>,
<viro@...iv.linux.org.uk>, <bharrosh@...asas.com>,
<devel@...nvz.org>
Subject: Re: [RFC PATCH] kmod: add ability to swap root in usermode helper
20.05.2013 12:42, Jeff Layton пишет:
> On Mon, 20 May 2013 11:00:37 +0400
> Stanislav Kinsbursky <skinsbursky@...allels.com> wrote:
>
>> Usermode helper executes all binaries in global "init" root context. This
>> doesn't allow to call to call the binary from other root (for example in a
>> container).
>> Currently, containerized NFS server requires an ability to execute a binary in
>> a other context, than "init" root (UMH is used for client recovery tracking).
>> This patch adds root swap to ____call_usermodehelper(), if non-NULL root was
>> passed as a part of subprocess_info data, and a call_usermodehelper_root()
>> helper, which accept root as a parameter. Root path reference must be hold by
>> the caller, since it will be on UMH thread exit.
>>
>
> I assume you mean that it will be put on thread exit.
>
Yes, sure. Sorry.
>> Signed-off-by: Stanislav Kinsbursky <skinsbursky@...allels.com>
>> ---
>> include/linux/kmod.h | 9 +++++++++
>> kernel/kmod.c | 41 ++++++++++++++++++++++++++++++++++-------
>> 2 files changed, 43 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/kmod.h b/include/linux/kmod.h
>> index 0555cc6..0b041c7 100644
>> --- a/include/linux/kmod.h
>> +++ b/include/linux/kmod.h
>> @@ -64,12 +64,21 @@ struct subprocess_info {
>> int (*init)(struct subprocess_info *info, struct cred *new);
>> void (*cleanup)(struct subprocess_info *info);
>> void *data;
>> + struct path *root;
>> };
>>
>> extern int
>> +call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
>> + struct path *root);
>> +extern int
>> call_usermodehelper(char *path, char **argv, char **envp, int wait);
>>
>> extern struct subprocess_info *
>> +call_usermodehelper_setup_root(char *path, char **argv, char **envp, gfp_t gfp_mask,
>> + int (*init)(struct subprocess_info *info, struct cred *new),
>> + void (*cleanup)(struct subprocess_info *), void *data,
>> + struct path *root);
>> +extern struct subprocess_info *
>> call_usermodehelper_setup(char *path, char **argv, char **envp, gfp_t gfp_mask,
>> int (*init)(struct subprocess_info *info, struct cred *new),
>> void (*cleanup)(struct subprocess_info *), void *data);
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index 1296e72..1b41f2c 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -39,6 +39,7 @@
>> #include <linux/rwsem.h>
>> #include <linux/ptrace.h>
>> #include <linux/async.h>
>> +#include <linux/fs_struct.h>
>> #include <asm/uaccess.h>
>>
>> #include <trace/events/module.h>
>> @@ -215,6 +216,9 @@ static int ____call_usermodehelper(void *data)
>> */
>> set_user_nice(current, 0);
>>
>> + if (sub_info->root)
>> + set_fs_root(current->fs, sub_info->root);
>> +
>> retval = -ENOMEM;
>> new = prepare_kernel_cred(current);
>> if (!new)
>> @@ -505,7 +509,7 @@ static void helper_unlock(void)
>> }
>>
>> /**
>> - * call_usermodehelper_setup - prepare to call a usermode helper
>> + * call_usermodehelper_setup_root - prepare to call a usermode helper
>> * @path: path to usermode executable
>> * @argv: arg vector for process
>> * @envp: environment for process
>> @@ -513,6 +517,7 @@ static void helper_unlock(void)
>> * @cleanup: a cleanup function
>> * @init: an init function
>> * @data: arbitrary context sensitive data
>> + * @root: fs root to swap to before binary execution
>> *
>> * Returns either %NULL on allocation failure, or a subprocess_info
>> * structure. This should be passed to call_usermodehelper_exec to
>> @@ -527,11 +532,11 @@ static void helper_unlock(void)
>> * Function must be runnable in either a process context or the
>> * context in which call_usermodehelper_exec is called.
>> */
>
> It would be best to spell out the need for the caller to hold a
> reference in the above kerneldoc comment, since that's what people will
> look at when they write new code that calls this.
>
Sound reasonable, thanks.
>> -struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> +struct subprocess_info *call_usermodehelper_setup_root(char *path, char **argv,
>> char **envp, gfp_t gfp_mask,
>> int (*init)(struct subprocess_info *info, struct cred *new),
>> void (*cleanup)(struct subprocess_info *info),
>> - void *data)
>> + void *data, struct path *root)
>> {
>> struct subprocess_info *sub_info;
>> sub_info = kzalloc(sizeof(struct subprocess_info), gfp_mask);
>> @@ -546,9 +551,22 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> sub_info->cleanup = cleanup;
>> sub_info->init = init;
>> sub_info->data = data;
>> +
>> + sub_info->root = root;
>> out:
>> return sub_info;
>> }
>> +EXPORT_SYMBOL(call_usermodehelper_setup_root);
>> +
>
> nit: do we really need a new exported helper function here? There
> aren't that many callers of call_usermodehelper_setup, so you could
> just add the argument and fix up the existing callers.
>
Or maybe even define call_usermodehelper_setup as a macro?
>> +struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
>> + char **envp, gfp_t gfp_mask,
>> + int (*init)(struct subprocess_info *info, struct cred *new),
>> + void (*cleanup)(struct subprocess_info *info),
>> + void *data)
>> +{
>> + return call_usermodehelper_setup_root(path, argv, envp, gfp_mask, init,
>> + cleanup, data, NULL);
>> +}
>> EXPORT_SYMBOL(call_usermodehelper_setup);
>>
>> /**
>> @@ -617,7 +635,7 @@ unlock:
>> EXPORT_SYMBOL(call_usermodehelper_exec);
>>
>> /**
>> - * call_usermodehelper() - prepare and start a usermode application
>> + * call_usermodehelper_root() - prepare and start a usermode application
>> * @path: path to usermode executable
>> * @argv: arg vector for process
>> * @envp: environment for process
>> @@ -625,24 +643,33 @@ EXPORT_SYMBOL(call_usermodehelper_exec);
>> * when UMH_NO_WAIT don't wait at all, but you get no useful error back
>> * when the program couldn't be exec'ed. This makes it safe to call
>> * from interrupt context.
>> + * @root: fs root to swap to before binary execution
>> *
>> * This function is the equivalent to use call_usermodehelper_setup() and
>> * call_usermodehelper_exec().
>> */
>
> I'd also spell out the need for the caller to hold an extra reference
> to @root here.
>
Right, thanks.
>> -int call_usermodehelper(char *path, char **argv, char **envp, int wait)
>> +int call_usermodehelper_root(char *path, char **argv, char **envp, int wait,
>> + struct path *root)
>> {
>> struct subprocess_info *info;
>> gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>>
>> - info = call_usermodehelper_setup(path, argv, envp, gfp_mask,
>> - NULL, NULL, NULL);
>> + info = call_usermodehelper_setup_root(path, argv, envp, gfp_mask,
>> + NULL, NULL, NULL, root);
>> if (info == NULL)
>> return -ENOMEM;
>>
>> return call_usermodehelper_exec(info, wait);
>> }
>> +EXPORT_SYMBOL(call_usermodehelper_root);
>> +
>> +int call_usermodehelper(char *path, char **argv, char **envp, int wait)
>> +{
>> + return call_usermodehelper_root(path, argv, envp, wait, NULL);
>> +}
>> EXPORT_SYMBOL(call_usermodehelper);
>>
>> +
>> static int proc_cap_handler(struct ctl_table *table, int write,
>> void __user *buffer, size_t *lenp, loff_t *ppos)
>> {
>>
>
> Looks reasonable otherwise, you can add my Acked-by when you fix up the
> comments. Nice work...
>
--
Best regards,
Stanislav Kinsbursky
--
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/
Powered by blists - more mailing lists