diff --git a/runtime/staprun/staprun.c b/runtime/staprun/staprun.c index d72e335..ca245e3 100644 --- a/runtime/staprun/staprun.c +++ b/runtime/staprun/staprun.c @@ -119,19 +119,7 @@ static int enable_uprobes(void) if (run_as(0, uid, gid, argv[0], argv) == 0) return 0; - /* - * TODO: If user can't setresuid to root here, staprun will exit. - * Is there a situation where that would fail but the subsequent - * attempt to insert_module() would succeed? - */ - dbug(2, "Inserting uprobes module from /lib/modules, if any.\n"); - i = 0; - argv[i++] = "/sbin/modprobe"; - argv[i++] = "-q"; - argv[i++] = "uprobes"; - argv[i] = NULL; - if (run_as(0, 0, 0, argv[0], argv) == 0) - return 0; + /* NB: don't use /sbin/modprobe, without more env. sanitation. */ /* This module may be signed, so use insert_module to load it. */ snprintf (runtimeko, sizeof(runtimeko), "%s/uprobes/uprobes.ko", @@ -190,9 +178,16 @@ static int remove_module(const char *name, int verb) return 0; } - /* We could call init_ctl_channel / close_ctl_channel here, as a heuristic - to determine whether the module is being used by some other stapio process. - However, delete_module() does basically the same thing. */ + /* We call init_ctl_channel/close_ctl_channel to check whether + the module is a systemtap-built one (having the right files), + and that it's already unattached (because otherwise it'd EBUSY + the opens. */ + ret = init_ctl_channel (name, 0); + if (ret < 0) { + err("Error, '%s' is not a zombie systemtap module.\n", name); + return ret; + } + close_ctl_channel (); dbug(2, "removing module %s\n", name); STAP_PROBE1(staprun, remove__module, name); @@ -227,7 +222,7 @@ int init_staprun(void) without first removing the kernel module. This would block a subsequent rerun attempt. So here we gingerly try to unload it first. */ - int ret = delete_module (modname, O_NONBLOCK); + int ret = remove_module (modname, 0); err("Retrying, after attempted removal of module %s (rc %d)\n", modname, ret); /* Then we try an insert a second time. */ if (insert_stap_module() < 0) diff --git a/README.security b/README.security index 124ad8d..998bf3d 100644 --- a/README.security +++ b/README.security @@ -15,7 +15,7 @@ following: * the root user; - * a member of the 'stapdev' group; or + * a member of both 'stapdev' and 'stapusr' groups; or * a member of the 'stapusr' group. Members of the stapusr group can only use modules located in the /lib/modules/VERSION/systemtap @@ -23,8 +23,8 @@ following: directory must be owned by root and not be world writable. So, there are two classes of users: systemtap developers (the root user -and members of the stapdev group) and systemtap users (members of the -stapusr group). Systemtap developers can compile and run any +and members of the stapdev/stapusr groups) and systemtap users (members of +only the stapusr group). Systemtap developers can compile and run any systemtap script. Systemtap users can only run "approved" pre-compiled modules located in /lib/modules/VERSION/systemtap. diff --git a/staprun.8 b/staprun.8 index f4a5a08..7523031 100644 --- a/staprun.8 +++ b/staprun.8 @@ -205,14 +205,14 @@ structures and potentially private user information. See the .IR stap (1) manual page for additional information on safety and security. .PP -To increase system security, only the root user and members of the -.I stapdev -group can use +To increase system security, only the root user and members of both +.I stapdev " and " staprun +groups can use .I staprun to insert systemtap modules (or attach to existing ones). Members of the .I stapusr -group can use +group only can use .I staprun to insert or remove systemtap modules (or attach to existing systemtap modules) under the following conditions: diff --git a/runtime/staprun/ctl.c b/runtime/staprun/ctl.c index 335006e..8baf0db 100644 --- a/runtime/staprun/ctl.c +++ b/runtime/staprun/ctl.c @@ -27,6 +27,9 @@ int init_ctl_channel(const char *name, int verb) return -2; } + if (access(buf, R_OK|W_OK) != 0) + return -5; + control_channel = open(buf, O_RDWR); dbug(2, "Opened %s (%d)\n", buf, control_channel); if (control_channel < 0) {