From e7d4a0cfcadff52d896d6017ec02403a948241d9 Mon Sep 17 00:00:00 2001 From: Yaroslav Bolyukin Date: Wed, 25 Sep 2024 23:34:35 +0200 Subject: [PATCH 1/6] fix: shmoverride hooks in presence of other constructors calling fstat --- shmoverride/shmoverride.c | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index d399905d..3cd95362 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -72,6 +72,8 @@ static int (*real_munmap) (void *shmaddr, size_t len); static int (*real_fstat64) (VER_ARG int fd, struct stat64 *buf); static int (*real_fstat)(VER_ARG int fd, struct stat *buf); +static int try_init(void); + static struct stat global_buf; static int gntdev_fd = -1; @@ -84,7 +86,7 @@ static int xc_hnd; static xengnttab_handle *xgt; static char __shmid_filename[SHMID_FILENAME_LEN]; static char *shmid_filename = NULL; -static int idfd = -1, display = -1; +static int idfd = -1, display = -1, init_called = 0; static uint8_t *mmap_mfns(struct shm_args_hdr *shm_args) { uint8_t *map; @@ -151,6 +153,8 @@ ASM_DEF(void *, mmap, real_fstat = FSTAT; } + try_init(); + #if defined MAP_ANON && defined MAP_ANONYMOUS && (MAP_ANONYMOUS) != (MAP_ANON) # error header bug (def mismatch) #endif @@ -217,6 +221,9 @@ ASM_DEF(int, munmap, void *addr, size_t len) { if (len > SIZE_MAX - XC_PAGE_SIZE) abort(); + + try_init(); + const uintptr_t addr_int = (uintptr_t)addr; const uintptr_t rounded_addr = addr_int & ~(uintptr_t)(XC_PAGE_SIZE - 1); return real_munmap((void *)rounded_addr, len + (addr_int - rounded_addr)); @@ -438,6 +445,7 @@ static int assign_off(off_t *off) { #define STAT(id) \ ASM_DEF(int, f ## id, int filedes, struct id *buf) { \ + try_init(); \ int res = real_f ## id(VER filedes, buf); \ if (res || \ !S_ISCHR(buf->st_mode) || \ @@ -454,6 +462,7 @@ STAT(stat64) #ifdef _STAT_VER #define STAT(id) \ ASM_DEF(int, __fx ## id, int ver, int filedes, struct id *buf) { \ + try_init(); \ if (ver != _STAT_VER) { \ fprintf(stderr, \ "Wrong _STAT_VER: got %d, expected %d, libc has incompatibly changed\n", \ @@ -467,8 +476,13 @@ STAT(stat64) #undef STAT #endif -int __attribute__ ((constructor)) initfunc(void) +static int try_init(void) { + // Ideally it is being called in constructor, if something is calling this before + // constructor - we're assuming it is not multi-threaded code. + if (__builtin_expect(init_called, 1)) return 0; + init_called = 1; + unsetenv("LD_PRELOAD"); fprintf(stderr, "shmoverride constructor running\n"); dlerror(); @@ -581,6 +595,10 @@ int __attribute__ ((constructor)) initfunc(void) shm_args = NULL; return 0; } +int __attribute__ ((constructor)) initfunc(void) +{ + return try_init(); +} int __attribute__ ((destructor)) descfunc(void) { From a87ee64aea0a120d1a7fcf372bd829d27b9a5852 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 14 Oct 2024 06:12:47 +0200 Subject: [PATCH 2/6] Add Xwayland wrapper similar to the one for Xorg Add a wrapper that sets LD_PRELOAD=.../shmoverride.so. Since suid binaries are not involved with Xwayland, it can be a simple shell script. Place the wrapper as Xwayland in /usr/libexec/qubes/wrappers. Processes that are supposed to use it need to have /usr/libexec/qubes/wrappers prepended to PATH. QubesOS/qubes-issues#8515 --- Makefile | 8 +++++++- debian/qubes-gui-daemon.install | 1 + rpm_spec/gui-daemon.spec.in | 1 + shmoverride/.gitignore | 2 +- shmoverride/Makefile | 7 +++++-- shmoverride/Xwayland-wrapper.in | 4 ++++ 6 files changed, 19 insertions(+), 4 deletions(-) create mode 100755 shmoverride/Xwayland-wrapper.in diff --git a/Makefile b/Makefile index 54e36d99..56dea3f1 100644 --- a/Makefile +++ b/Makefile @@ -35,7 +35,9 @@ selinux_policies ::= qubes-gui-daemon.pp all_targets := gui-daemon/qubes-guid gui-daemon/qubes-guid.1 \ shmoverride/shmoverride.so \ - shmoverride/X-wrapper-qubes pulse/pacat-simple-vchan \ + shmoverride/X-wrapper-qubes \ + shmoverride/Xwayland-wrapper \ + pulse/pacat-simple-vchan \ screen-layout-handler/watch-screen-layout-changes @@ -52,6 +54,9 @@ shmoverride/shmoverride.so: shmoverride/X-wrapper-qubes: (cd shmoverride; $(MAKE) X-wrapper-qubes) +shmoverride/Xwayland-wrapper: + (cd shmoverride; $(MAKE) Xwayland-wrapper) + pulse/pacat-simple-vchan: $(MAKE) -C pulse pacat-simple-vchan @@ -67,6 +72,7 @@ install: install -D pulse/pacat-simple-vchan $(DESTDIR)/usr/bin/pacat-simple-vchan install -D shmoverride/X-wrapper-qubes $(DESTDIR)/usr/bin/X-wrapper-qubes install -D shmoverride/shmoverride.so $(DESTDIR)$(LIBDIR)/qubes-gui-daemon/shmoverride.so + install -D shmoverride/Xwayland-wrapper $(DESTDIR)/usr/libexec/qubes/wrappers/Xwayland install -D -m 0644 gui-daemon/guid.conf $(DESTDIR)/etc/qubes/guid.conf install -D gui-daemon/qubes-localgroup.sh $(DESTDIR)/etc/X11/xinit/xinitrc.d/qubes-localgroup.sh install -D -m 0644 common/90-default-gui-daemon.policy $(DESTDIR)/etc/qubes/policy.d/90-default-gui-daemon.policy diff --git a/debian/qubes-gui-daemon.install b/debian/qubes-gui-daemon.install index ddf9ceb0..049ee252 100644 --- a/debian/qubes-gui-daemon.install +++ b/debian/qubes-gui-daemon.install @@ -1,6 +1,7 @@ usr/bin/X.qubes usr/bin/qubes-guid usr/libexec/qubes/watch-screen-layout-changes +usr/libexec/qubes/wrappers/Xwayland usr/share/man/man1/qubes-guid.1 usr/lib/*/qubes-gui-daemon/shmoverride.so usr/lib/qubes/icon-receiver diff --git a/rpm_spec/gui-daemon.spec.in b/rpm_spec/gui-daemon.spec.in index 961a0880..6e0f2e4a 100644 --- a/rpm_spec/gui-daemon.spec.in +++ b/rpm_spec/gui-daemon.spec.in @@ -178,6 +178,7 @@ rm -f %{name}-%{version} /etc/xdg/autostart/qubes-icon-receiver.desktop /etc/X11/xinit/xinitrc.d/qubes-localgroup.sh /usr/libexec/qubes/watch-screen-layout-changes +/usr/libexec/qubes/wrappers/Xwayland /usr/lib/qubes/icon-receiver %config %{_sysconfdir}/qubes-rpc/qubes.WindowIconUpdater %config %{_sysconfdir}/qubes/rpc-config/qubes.WindowIconUpdater diff --git a/shmoverride/.gitignore b/shmoverride/.gitignore index 979bacea..9352006f 100644 --- a/shmoverride/.gitignore +++ b/shmoverride/.gitignore @@ -1,3 +1,3 @@ X_wrapper_qubes shmoverride.so - +Xwayland-wrapper diff --git a/shmoverride/Makefile b/shmoverride/Makefile index 854742fb..a75441b1 100644 --- a/shmoverride/Makefile +++ b/shmoverride/Makefile @@ -33,7 +33,7 @@ extra_cflags := -g -O2 -I../include/ -fPIC -Wall -Wextra -Werror \ -I../include -fvisibility=hidden -pthread CC=gcc -all: shmoverride.so X-wrapper-qubes +all: shmoverride.so X-wrapper-qubes Xwayland-wrapper shmoverride.so: shmoverride.o ./list.o $(CC) $(CFLAGS) $(extra_cflags) -shared -o shmoverride.so \ @@ -43,8 +43,11 @@ vpath %.c ../common X-wrapper-qubes: X-wrapper-qubes.o +Xwayland-wrapper: Xwayland-wrapper.in + sed -e "s,@SHMOVERRIDE_LIB_PATH@,$(LIBDIR)/qubes-gui-daemon/shmoverride.so," < $< > $@ + clean: - rm -f ./*~ ./*.o shmoverride.so X-wrapper-qubes + rm -f ./*~ ./*.o shmoverride.so X-wrapper-qubes Xwayland-wrapper %.o: %.c Makefile $(CC) -MD -MP -MF $@.dep -c -o $@ $(extra_cflags) $(CFLAGS) $< diff --git a/shmoverride/Xwayland-wrapper.in b/shmoverride/Xwayland-wrapper.in new file mode 100755 index 00000000..e9139e00 --- /dev/null +++ b/shmoverride/Xwayland-wrapper.in @@ -0,0 +1,4 @@ +#!/bin/sh + +export PATH="${PATH#*/wrappers:}" +exec env LD_PRELOAD="@SHMOVERRIDE_LIB_PATH@" Xwayland "$@" From 9efde41042ba666ca20998535abfb21218efa3dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 14 Oct 2024 15:53:58 +0200 Subject: [PATCH 3/6] shmoverride: do not remove the shmid file if taking the lock fails If shmoverride init fails to take the lock, it likely means it is used by currently running process - do _not_ remove the file in such a case. This may happen if LD_PRELOAD leaks to child processes (which seems to happend on Xwayland when shmoverride is linked with -z initfirst). But also, if X server is started several times for some reason (for example user calls it manually), it will abort, but not early enough to skip shmoverride init. QubesOS/qubes-issues#8515 --- shmoverride/shmoverride.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index 3cd95362..ce71a29b 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -533,24 +533,26 @@ static int try_init(void) fputs("snprintf() failed!\n", stderr); abort(); } - shmid_filename = __shmid_filename; - fprintf(stderr, "shmoverride: running with shm file %s\n", shmid_filename); + fprintf(stderr, "shmoverride: running with shm file %s\n", __shmid_filename); /* Try to lock the shm.id file (don't rely on whether it exists, a previous * process might have crashed). */ - idfd = open(shmid_filename, O_RDWR | O_CLOEXEC | O_CREAT | O_NOCTTY, 0600); + idfd = open(__shmid_filename, O_RDWR | O_CLOEXEC | O_CREAT | O_NOCTTY, 0600); if (idfd < 0) { fprintf(stderr, "shmoverride opening %s: %s\n", - shmid_filename, strerror(errno)); + __shmid_filename, strerror(errno)); goto cleanup; } if (flock(idfd, LOCK_EX | LOCK_NB) < 0) { fprintf(stderr, "shmoverride flock %s: %s\n", - shmid_filename, strerror(errno)); + __shmid_filename, strerror(errno)); /* There is probably an alive process holding the file, give up. */ goto cleanup; } + /* Save shmid file for cleanup only after taking the lock */ + shmid_filename = __shmid_filename; + if (ftruncate(idfd, SHM_ARGS_SIZE) < 0) { perror("shmoverride ftruncate"); goto cleanup; From cb8f28d340bb28cee59c677082878e2ded318b95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 14 Oct 2024 19:43:43 +0200 Subject: [PATCH 4/6] shmoverride: move display check earlier Look for display number as the first thing after stuff necessary to work in passive mode. This will be relevant for filtering which process gets shmoverride enabled. QubesOS/qubes-issues#8515 --- shmoverride/shmoverride.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index ce71a29b..9302b15e 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -498,7 +498,12 @@ static int try_init(void) } else if (!(real_munmap = dlsym(RTLD_NEXT, "munmap"))) { fprintf(stderr, "shmoverride: no munmap?: %s\n", dlerror()); abort(); - } else if ((gntdev_fd = open("/dev/xen/gntdev", O_PATH | O_CLOEXEC | O_NOCTTY)) == -1) { + } + + if ((display = get_display()) < 0) + goto cleanup; + + if ((gntdev_fd = open("/dev/xen/gntdev", O_PATH | O_CLOEXEC | O_NOCTTY)) == -1) { perror("open /dev/xen/gntdev"); goto cleanup; } else if (real_fstat(VER gntdev_fd, &global_buf)) { @@ -508,6 +513,7 @@ static int try_init(void) fprintf(stderr, "/dev/xen/gntdev is not a character special file"); goto cleanup; } + #ifdef XENCTRL_HAS_XC_INTERFACE xc_hnd = xc_interface_open(NULL, NULL, 0); if (!xc_hnd) { @@ -525,9 +531,6 @@ static int try_init(void) goto cleanup; // Allow it to run when not under Xen. } - if ((display = get_display()) < 0) - goto cleanup; - if ((unsigned int)snprintf(__shmid_filename, sizeof __shmid_filename, SHMID_FILENAME_PREFIX "%d", display) >= sizeof __shmid_filename) { fputs("snprintf() failed!\n", stderr); From 7fac094981bed00a599ce66b163ff8152b3e2283 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 14 Oct 2024 20:33:43 +0200 Subject: [PATCH 5/6] shmoverride: allow applying special behavior only to specific programs This allows setting LD_PRELOAD earlier for some parent process, and have it actually applied to selected child process. This is especially helpful for Xwayland that is started by the wayland compositor and rarely has an option to change binary path (contrary to display managers starting Xorg). This feature allows setting LD_PRELOAD on the compositor itself, and have it affected only Xwayland process. Note, this feature does not work if shmoverride is linked with -z initfirst, as getenv()/unsetenv() are not working in that case. QubesOS/qubes-issues#8515 --- shmoverride/shmoverride.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/shmoverride/shmoverride.c b/shmoverride/shmoverride.c index 9302b15e..722ad0d0 100644 --- a/shmoverride/shmoverride.c +++ b/shmoverride/shmoverride.c @@ -357,7 +357,7 @@ static bool parse_display_name(const char *const ptr, int *display_num) return true; } -static int get_display(void) +static int get_display(const char *progname_allowlist) { ssize_t res; int fd, rc = -1, display = 0; @@ -376,6 +376,7 @@ static int get_display(void) return rc; } + bool argv0 = progname_allowlist != NULL; bool skip = true; /* Skip argv[0] (the program name) */ while(true) { errno = 0; @@ -389,6 +390,35 @@ static int get_display(void) size_t length = (size_t)res; assert(ptr && ptr[length] == '\0'); + if (argv0) { + char *current_item, *next_item; + char *allowlist_copy = strdup(progname_allowlist); + char *progname = strrchr(ptr, '/'); + + if (!allowlist_copy) { + perror("allowlist copy"); + goto cleanup; + } + + if (progname) + /* skip '/' */ + progname++; + else + progname = ptr; + + next_item = allowlist_copy; + while ((current_item = strsep(&next_item, " ")) != NULL) { + if (strcmp(current_item, progname) == 0) + break; + } + free(allowlist_copy); + /* abort if not found */ + if (!current_item) { + fprintf(stderr, "skipping shmoverride in %s\n", progname); + goto cleanup; + } + } + /* * Skip option arguments. Some options take more than one argument, * but the extra arguments are always optional and display names @@ -483,7 +513,6 @@ static int try_init(void) if (__builtin_expect(init_called, 1)) return 0; init_called = 1; - unsetenv("LD_PRELOAD"); fprintf(stderr, "shmoverride constructor running\n"); dlerror(); if (!(real_mmap = dlsym(RTLD_NEXT, "mmap64"))) { @@ -500,9 +529,11 @@ static int try_init(void) abort(); } - if ((display = get_display()) < 0) + if ((display = get_display(getenv("SHMOVERRIDE_PROGLIST"))) < 0) goto cleanup; + unsetenv("LD_PRELOAD"); + if ((gntdev_fd = open("/dev/xen/gntdev", O_PATH | O_CLOEXEC | O_NOCTTY)) == -1) { perror("open /dev/xen/gntdev"); goto cleanup; From 5c6b39e06e6c8241527393f32553a08991236f27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?= Date: Mon, 14 Oct 2024 20:43:08 +0200 Subject: [PATCH 6/6] shmoverride: extend documentation about LD_PRELOAD usage --- shmoverride/README | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/shmoverride/README b/shmoverride/README index 04f8cf30..5ea08fdc 100644 --- a/shmoverride/README +++ b/shmoverride/README @@ -20,3 +20,10 @@ it uses cmd_pages to determine which frames or grant pages should be mapped and from which domain. The munmap() implementation checks if the address is one that shmoverride.so had previously mapped, and if so, calls the appropriate Xen API functions to release the memory. + + +The LD_PRELOAD can be used in two ways: +1. Set directly for the Xorg/Xwayland process. For this, the package provides convenient wrappers: + - /usr/bin/X for Xorg + - /usr/libexec/qubes/wrappers/Xwayland for Xwayland (can be used by prepending /usr/libexec/qubes/wrappers to PATH for the wayland compositor process) +2. Set for some parent process and use SHMOVERRIDE_PROGLIST="Xorg Xwayland" (space separated list of program basenames) to select which process should be affected.