As part of a LSM I am writing, I need to call a user-space program and
check its return status. I found the call_usermodehelper function and
call it with the wait flag set, but I cannot get a non-zero return
status of the program to propagate into the kernel. If I try to run a
non-existent program then call_usermodehelper returns -1, so at least
some errors propagate properly. I have attached a trivial LSM test
program that hooks inode_rename, runs /bin/false and prints the return
status of call_usermodehelper.
This is with kernel 2.6.0-test5-mm3 compiled on an up to date Debian
unstable.
For simplicity I have also attached a patch to security/Makefile to
build this test LSM as a kernel module.
Before I break out UML or the kernel debugger, does anyone have any
ideas what I am doing wrong?
Thanks,
Omen
--
There is much Obi-Wan did not tell you.
Omen Wild <[email protected]> wrote:
>
> I found the call_usermodehelper function and
> call it with the wait flag set, but I cannot get a non-zero return
> status of the program to propagate into the kernel.
This might fix it.
25-akpm/kernel/exit.c | 21 +++++++++++++++++----
25-akpm/kernel/kmod.c | 2 +-
2 files changed, 18 insertions(+), 5 deletions(-)
diff -puN kernel/kmod.c~call_usermodehelper-retval-fix kernel/kmod.c
--- 25/kernel/kmod.c~call_usermodehelper-retval-fix Fri Sep 19 11:14:47 2003
+++ 25-akpm/kernel/kmod.c Fri Sep 19 11:20:14 2003
@@ -190,7 +190,7 @@ static int wait_for_helper(void *data)
/* We don't have a SIGCHLD signal handler, so this
* always returns -ECHILD, but the important thing is
* that it blocks. */
- sys_wait4(pid, NULL, 0, NULL);
+ sys_wait4(pid, &sub_info->retval, 0, NULL);
complete(sub_info->complete);
return 0;
diff -puN kernel/exit.c~call_usermodehelper-retval-fix kernel/exit.c
--- 25/kernel/exit.c~call_usermodehelper-retval-fix Fri Sep 19 11:16:59 2003
+++ 25-akpm/kernel/exit.c Fri Sep 19 11:20:10 2003
@@ -883,10 +883,17 @@ static int wait_task_zombie(task_t *p, u
retval = ru ? getrusage(p, RUSAGE_BOTH, ru) : 0;
if (!retval && stat_addr) {
+ int stat;
+
if (p->signal->group_exit)
- retval = put_user(p->signal->group_exit_code, stat_addr);
+ stat = p->signal->group_exit_code;
+ else
+ stat = p->exit_code;
+
+ if (current->mm)
+ retval = put_user(stat, stat_addr);
else
- retval = put_user(p->exit_code, stat_addr);
+ retval = __put_user(stat, stat_addr);
}
if (retval) {
p->state = TASK_ZOMBIE;
@@ -987,8 +994,14 @@ static int wait_task_stopped(task_t *p,
write_unlock_irq(&tasklist_lock);
retval = ru ? getrusage(p, RUSAGE_BOTH, ru) : 0;
- if (!retval && stat_addr)
- retval = put_user((exit_code << 8) | 0x7f, stat_addr);
+ if (!retval && stat_addr) {
+ int stat = (exit_code << 8) | 0x7f;
+
+ if (current->mm)
+ retval = put_user(stat, stat_addr);
+ else
+ retval = __put_user(stat, stat_addr);
+ }
if (!retval)
retval = p->pid;
put_task_struct(p);
_
Quoting Andrew Morton <[email protected]> on Fri, Sep 19 11:21:
>
> This might fix it.
Hmmm, that did not fix it for me. No change in behavior.
--
There are more ways into the woods than out.
Omen Wild <[email protected]> wrote:
>
> Quoting Andrew Morton <[email protected]> on Fri, Sep 19 11:21:
> >
> > This might fix it.
>
> Hmmm, that did not fix it for me. No change in behavior.
>
Curses, foiled again.
I agree that as long as we are supporting synchronous callouts we should be
correctly returning the exit code. I'll work on it a bit.
Andrew Morton <[email protected]> wrote:
> Omen Wild <[email protected]> wrote:
> >
> > I found the call_usermodehelper function and
> > call it with the wait flag set, but I cannot get a non-zero return
> > status of the program to propagate into the kernel.
>
> This might fix it.
I think you missed the why behind the comment just above your first change.
/* We don't have a SIGCHLD signal handler, so this
* always returns -ECHILD, but the important thing is
* that it blocks. */
- sys_wait4(pid, NULL, 0, NULL);
+ sys_wait4(pid, &sub_info->retval, 0, NULL);
The exit code notices that there is no signal handler for SIGCHILD and
does a fast exit, then we notice when woken up the child no longer exists.
Rusty discovered this back in June when we were trying to fix a checker
error on the wait call, and decided that at the time no one was using the
return value, hence the simpler fix.
http://linux.bkbits.net:8080/linux-2.5/[email protected]?nav=index.html|src/|src/kernel|related/kernel/kmod.c
milton
* Milton D. Miller II ([email protected]) wrote:
> Andrew Morton <[email protected]> wrote:
> > This might fix it.
>
> I think you missed the why behind the comment just above your first change.
Anything wrong with just setting a SIG_DFL handler? W.R.T. the kernel
pointer, either Andrew's patch which does put_user/__put_user depending
on context, or some ugly set_fs() should work. This simplistic approach
works for me, thoughts?
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
--- linux-2.6.0-test5-mm4/kernel/kmod.c 2003-09-08 12:49:59.000000000 -0700
+++ 2.6.0-test5-mm4/kernel/kmod.c 2003-09-25 11:34:59.000000000 -0700
@@ -181,16 +181,24 @@
{
struct subprocess_info *sub_info = data;
pid_t pid;
+ struct k_sigaction sa;
+
+ sa.sa.sa_handler = SIG_DFL;
+ sa.sa.sa_flags = 0;
+ siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
+ do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
sub_info->retval = 0;
pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
if (pid < 0)
sub_info->retval = pid;
- else
- /* We don't have a SIGCHLD signal handler, so this
- * always returns -ECHILD, but the important thing is
- * that it blocks. */
- sys_wait4(pid, NULL, 0, NULL);
+ else {
+ mm_segment_t old_fs;
+ old_fs = get_fs();
+ set_fs(KERNEL_DS);
+ sys_wait4(pid, &sub_info->retval, 0, NULL);
+ set_fs(old_fs);
+ }
complete(sub_info->complete);
return 0;
Chris Wright <[email protected]> wrote:
>
> Anything wrong with just setting a SIG_DFL handler?
Seems that any time we make a change here it looks fine, tests out fine,
and explodes messily three weeks later.
> W.R.T. the kernel
> pointer, either Andrew's patch which does put_user/__put_user depending
> on context, or some ugly set_fs() should work. This simplistic approach
> works for me, thoughts?
Yes, set_fs() is better.
* Andrew Morton ([email protected]) wrote:
> Chris Wright <[email protected]> wrote:
> >
> > Anything wrong with just setting a SIG_DFL handler?
>
> Seems that any time we make a change here it looks fine, tests out fine,
> and explodes messily three weeks later.
Heh. Care to try it? ;-)
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net