rpms
/
systemtap
7
0
Fork 0
Code Issues Pull Requests Releases Activity
systemtap/SOURCES/rhbz1902696.patch

218 lines
8.6 KiB
Diff
Raw Blame History

commit e3d03db82853049f65f16dc40c03f3f7f617ffb5
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Sun Dec 13 21:05:23 2020 -0500
PR23512: fix staprun/stapio operation via less-than-root privileges
Commit 7615cae790c899bc8a82841c75c8ea9c6fa54df3 for PR26665 introduced
a regression in handling stapusr/stapdev/stapsys gid invocation of
staprun/stapio. This patch simplifies the relevant code in
staprun/ctl.c, init_ctl_channel(), to rely on openat/etc. to populate
and use the relay_basedir_fd as much as possible. Also, we now avoid
unnecessary use of access(), which was checking against the wrong
(real rather than effective) uid/gid.
diff --git a/staprun/ctl.c b/staprun/ctl.c
index 4be68af..da3417b 100644
--- a/staprun/ctl.c
+++ b/staprun/ctl.c
@@ -14,111 +14,70 @@
#define CTL_CHANNEL_NAME ".cmd"
+
+#ifndef HAVE_OPENAT
+#error "need openat"
+#endif
+
+
+// This function does multiple things:
+//
+// 1) if needed, open the running module's directory (the one that
+// contains .ctl), stash fd in relay_basedir_fd; this will be
+// passed to stapio children via -F$fd for privilege passing
+//
+// 2) (re)open the running module's .ctl file, stash fd in the
+// control_channel global; this will be used all over the place.
+//
+// Return 0 on success.
+//
+// See also PR14245, PR26665, RHBZ1902696 = PR23512
+//
int init_ctl_channel(const char *name, int verb)
{
- char buf[PATH_MAX] = ""; // the .ctl file name
- char buf2[PATH_MAX] = ""; // other tmp stuff
- struct statfs st;
-
(void) verb;
- if (0) goto out; /* just to defeat gcc warnings */
- /* Before trying to open the control channel, make sure it
- * isn't already open. */
- close_ctl_channel();
+ // Already got them both?
+ if (control_channel >= 0 && relay_basedir_fd >= 0)
+ return 0;
-#ifdef HAVE_OPENAT
- if (relay_basedir_fd >= 0) {
- strncpy(buf, CTL_CHANNEL_NAME, PATH_MAX - 1);
- control_channel = openat_cloexec(relay_basedir_fd,
- CTL_CHANNEL_NAME, O_RDWR, 0);
- dbug(2, "Opened %s (%d)\n", CTL_CHANNEL_NAME, control_channel);
+ // Need relay_basedir_fd .... ok try /sys/kernel/debug/systemtap/
+ if (relay_basedir_fd < 0) {
+ char buf[PATH_MAX] = "";
+ struct statfs st;
- /* NB: Extra real-id access check as below */
- if (faccessat(relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) != 0){
- close(control_channel);
- return -5;
- }
- if (control_channel >= 0)
- goto out; /* It's OK to bypass the [f]access[at] check below,
- since this would only occur the *second* time
- staprun tries this gig, or within unprivileged stapio. */
+ if (sprintf_chk(buf, "/sys/kernel/debug/systemtap/%s", name))
+ return -EINVAL;
+
+ if (statfs("/sys/kernel/debug", &st) == 0 && (int)st.f_type == (int)DEBUGFS_MAGIC)
+ relay_basedir_fd = open (buf, O_DIRECTORY | O_RDONLY);
}
- /* PR14245, NB: we fall through to /sys ... /proc searching,
- in case the relay_basedir_fd option wasn't given (i.e., for
- early in staprun), or if errors out for some reason. */
-#endif
-
- // See if we have the .ctl file in debugfs
- if (sprintf_chk(buf2, "/sys/kernel/debug/systemtap/%s/%s",
- name, CTL_CHANNEL_NAME))
- return -1;
- if (statfs("/sys/kernel/debug", &st) == 0 && (int)st.f_type == (int)DEBUGFS_MAGIC &&
- (access (buf2, W_OK)==0)) {
- /* PR14245: allow subsequent operations, and if
- necessary, staprun->stapio forks, to reuse an fd for
- directory lookups (even if some parent directories have
- perms 0700. */
- strcpy(buf, buf2); // committed
+ // Still need relay_basedir_fd ... ok try /proc/systemtap/
+ if (relay_basedir_fd < 0) {
+ char buf[PATH_MAX] = "";
-#ifdef HAVE_OPENAT
- if (! sprintf_chk(buf2, "/sys/kernel/debug/systemtap/%s", name)) {
- relay_basedir_fd = open (buf2, O_DIRECTORY | O_RDONLY);
- }
-#endif
- }
-
- // PR26665: try /proc/systemtap/... also
- // (STP_TRANSPORT_1 used to use this for other purposes.)
- if (sprintf_chk(buf2, "/proc/systemtap/%s/%s",
- name, CTL_CHANNEL_NAME))
- return -1;
- if (relay_basedir_fd < 0 && (access(buf2, W_OK)==0)) {
- strcpy(buf, buf2); // committed
+ if (sprintf_chk(buf, "/proc/systemtap/%s", name))
+ return -EINVAL;
-#ifdef HAVE_OPENAT
- if (! sprintf_chk(buf2, "/proc/systemtap/%s", name)) {
- relay_basedir_fd = open (buf2, O_DIRECTORY | O_RDONLY);
- }
-#endif
+ relay_basedir_fd = open (buf, O_DIRECTORY | O_RDONLY);
}
- /* At this point, we have buf, which is the full path to the .ctl file,
- and we may have a relay_basedir_fd, which is useful to pass across
- staprun->stapio fork/execs. */
-
- control_channel = open_cloexec(buf, O_RDWR, 0);
- dbug(2, "Opened %s (%d)\n", buf, control_channel);
-
- /* NB: Even if open() succeeded with effective-UID permissions, we
- * need the access() check to make sure real-UID permissions are also
- * sufficient. When we run under the setuid staprun, effective and
- * real UID may not be the same. Specifically, we want to prevent
- * a local stapusr from trying to attach to a different stapusr's module.
- *
- * The access() is done *after* open() to avoid any TOCTOU-style race
- * condition. We believe it's probably safe either way, as the file
- * we're trying to access connot be modified by a typical user, but
- * better safe than sorry.
- */
-#ifdef HAVE_OPENAT
- if (control_channel >= 0 && relay_basedir_fd >= 0) {
- if (faccessat (relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) == 0)
- goto out;
- /* else fall through */
+ // Got relay_basedir_fd, need .ctl
+ if (relay_basedir_fd >= 0) {
+ // verify that the ctl file is accessible to our real uid/gid
+ if (faccessat(relay_basedir_fd, CTL_CHANNEL_NAME, R_OK|W_OK, 0) != 0)
+ return -EPERM;
+
+ control_channel = openat_cloexec(relay_basedir_fd,
+ CTL_CHANNEL_NAME, O_RDWR, 0);
}
-#endif
- if (control_channel >= 0 && access(buf, R_OK|W_OK) != 0) {
- close(control_channel);
- return -5;
- }
-out:
- if (control_channel < 0) {
+ // Fell through
+ if (relay_basedir_fd < 0 || control_channel < 0) {
err(_("Cannot attach to module %s control channel; not running?\n"),
name);
- return -3;
+ return -EINVAL;
}
return 0;
}
commit 1120422c2822be9e00d8d11cab3fb381d2ce0cce
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Sun Dec 13 21:19:15 2020 -0500
PR27067 <<< corrected bug# for previous commit
commit cd5b72a538a404011d27d86ff958355ac2c45b8d
Author: Frank Ch. Eigler <fche@redhat.com>
Date: Sun Jan 24 14:45:54 2021 -0500
PR27067: set procfs traceNN files' uid/gid too
commit e3d03db828 neglected to include the proper calls to set the
procfs traceNN files to the correct uid/gid ownership. With those
files left as uid/gid=0/0, stapio running with a user with
stapusr/stapdev privileges couldn't fopenat() those files. Now they
can again. This problem became obvious after commit 4706ab3ca5c0,
which makes STAP_TRANS_PROCFS the default.
diff --git a/runtime/transport/procfs.c b/runtime/transport/procfs.c
index 97a6e123a..69591a235 100644
--- a/runtime/transport/procfs.c
+++ b/runtime/transport/procfs.c
@@ -336,12 +336,14 @@ __stp_procfs_relay_create_buf_file_callback(const char *filename,
if (parent != _stp_procfs_module_dir_path.dentry)
goto out;
- pde = proc_create (filename, 0600,
+ pde = proc_create (filename, 0400,
_stp_procfs_module_dir,
& relay_procfs_operations);
if (pde == NULL)
goto out;
+ proc_set_user(pde, KUIDT_INIT(_stp_uid), KGIDT_INIT(_stp_gid));
+
rc = snprintf(fullpath, sizeof(fullpath), "/proc/systemtap/%s/%s",
THIS_MODULE->name, filename);
Reference in New Issue View Git Blame Copy Permalink