commit e77813ca9f4e0735fd0e3e2caae4d7d1ee436011 Author: Zbigniew Jędrzejewski-Szmek Date: Tue May 24 05:32:30 2016 -0400 Revert "rules: allow users to access frame buffer devices" (#3333) This reverts commit 483d8bbb4c0190f419bf9fba57fb0feb1a56bea6. In [1] Michel Dänzer and Daniel Vetter wrote: >> The scenario you describe isn't possible if the Wayland compositor >> directly uses the KMS API of /dev/dri/card*, but it may be possible if >> the Wayland compositor uses the fbdev API of /dev/fb* instead (e.g. if >> weston uses its fbdev backend). > > Yeah, if both weston and your screen grabber uses native fbdev API you can > now screenshot your desktop. And since fbdev has no concept of "current > owner of the display hw" like the drm master, I think this is not fixable. > At least not just in userspace. Also even with native KMS compositors > fbdev still doesn't have the concept of ownership, which is why it doesn't > bother clearing it's buffer before KMS takes over. I agree that this > should be reverted or at least hidden better. TBH, I think that privilege separation between processes running under the same UID is tenuous. Even with drm, in common setups any user process can ptrace the "current owner of the display" and call DROP_MASTER or do whatever. It *is* possible to prevent that, e.g. by disabling ptrace using yama.ptrace_scope, or selinux, and so on, but afaik this is not commonly done. E.g. all Fedora systems pull in elfutils-default-yama-scope.rpm through dependencies which sets yama.ptrace_scope=0. And even assuming that ptrace was disabled, it is trivial to modify files on disk, communicate through dbus, etc; there is just to many ways for a non-sandboxed process to interact maliciously with the display shell to close them all off. To achieve real protection, some sort of sandboxing must be implemented, and in that case there is no need to rely on access mode on the device files, since much more stringent measures have to be implemented anyway. The situation is similar for framebuffer devices. It is common to add framebuffer users to video group to allow them unlimited access to /dev/fb*. Using uaccess would be better solution in that case. Also, since there is no "current owner" limitation like in DRM, processes running under the same UID should be able to access /proc//fd/* and gain access to the devices. Nevertheless, weston implements a suid wrapper to access the devices and then drop privileges, and this patch would make this daemon pointless. So if the weston developers feel that this change reduces security, I prefer to revert it. [1] https://lists.freedesktop.org/archives/wayland-devel/2016-May/029017.html diff --git a/src/login/70-uaccess.rules b/src/login/70-uaccess.rules index 886c5bf..50dcd2e 100644 --- a/src/login/70-uaccess.rules +++ b/src/login/70-uaccess.rules @@ -42,9 +42,8 @@ SUBSYSTEM=="firewire", ATTR{units}=="*0x00b09d:0x00010*", TAG+="uaccess" SUBSYSTEM=="firewire", ATTR{units}=="*0x00a02d:0x010001*", TAG+="uaccess" SUBSYSTEM=="firewire", ATTR{units}=="*0x00a02d:0x014001*", TAG+="uaccess" -# DRI and frame buffer video devices +# DRI video devices SUBSYSTEM=="drm", KERNEL=="card*|renderD*", TAG+="uaccess" -SUBSYSTEM=="graphics", KERNEL=="fb*", TAG+="uaccess" # KVM SUBSYSTEM=="misc", KERNEL=="kvm", TAG+="uaccess" commit acc0269cad31d1aaef2034a055b34c07c88a353d Author: Christian Hesse Date: Thu May 26 15:57:37 2016 +0200 {machine,system}ctl: always pass &changes and &n_changes (#3350) We have to pass addresses of changes and n_changes to bus_deserialize_and_dump_unit_file_changes(). Otherwise we are hit by missing information (subsequent calls to unit_file_changes_add() to not add anything). Also prevent null pointer dereference in bus_deserialize_and_dump_unit_file_changes() by asserting. Fixes #3339 diff --git a/src/machine/machinectl.c b/src/machine/machinectl.c index 1165ab5..8e4ffa9 100644 --- a/src/machine/machinectl.c +++ b/src/machine/machinectl.c @@ -1602,6 +1602,8 @@ static int start_machine(int argc, char *argv[], void *userdata) { static int enable_machine(int argc, char *argv[], void *userdata) { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL, *reply = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + UnitFileChange *changes = NULL; + unsigned n_changes = 0; int carries_install_info = 0; const char *method = NULL; sd_bus *bus = userdata; @@ -1662,9 +1664,9 @@ static int enable_machine(int argc, char *argv[], void *userdata) { return bus_log_parse_error(r); } - r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL); + r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, &changes, &n_changes); if (r < 0) - return r; + goto finish; r = sd_bus_call_method( bus, @@ -1677,10 +1679,15 @@ static int enable_machine(int argc, char *argv[], void *userdata) { NULL); if (r < 0) { log_error("Failed to reload daemon: %s", bus_error_message(&error, -r)); - return r; + goto finish; } - return 0; + r = 0; + +finish: + unit_file_changes_free(changes, n_changes); + + return r; } static int match_log_message(sd_bus_message *m, void *userdata, sd_bus_error *error) { diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index f6559cd..f68c4a4 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -865,6 +865,11 @@ int bus_deserialize_and_dump_unit_file_changes(sd_bus_message *m, bool quiet, Un const char *type, *path, *source; int r; + /* changes is dereferenced when calling unit_file_dump_changes() later, + * so we have to make sure this is not NULL. */ + assert(changes); + assert(n_changes); + r = sd_bus_message_enter_container(m, SD_BUS_TYPE_ARRAY, "(sss)"); if (r < 0) return bus_log_parse_error(r); diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index b943c68..0500593 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -2058,6 +2058,8 @@ static int get_default(int argc, char *argv[], void *userdata) { static int set_default(int argc, char *argv[], void *userdata) { _cleanup_free_ char *unit = NULL; + UnitFileChange *changes = NULL; + unsigned n_changes = 0; int r; assert(argc >= 2); @@ -2068,13 +2070,8 @@ static int set_default(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to mangle unit name: %m"); if (install_client_side()) { - UnitFileChange *changes = NULL; - unsigned n_changes = 0; - r = unit_file_set_default(arg_scope, arg_root, unit, true, &changes, &n_changes); unit_file_dump_changes(r, "set default", changes, n_changes, arg_quiet); - unit_file_changes_free(changes, n_changes); - return r; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; @@ -2098,9 +2095,9 @@ static int set_default(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "Failed to set default target: %s", bus_error_message(&error, r)); - r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL); + r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, &changes, &n_changes); if (r < 0) - return r; + goto finish; /* Try to reload if enabled */ if (!arg_no_reload) @@ -2109,6 +2106,9 @@ static int set_default(int argc, char *argv[], void *userdata) { r = 0; } +finish: + unit_file_changes_free(changes, n_changes); + return r; } @@ -5650,6 +5650,8 @@ static int add_dependency(int argc, char *argv[], void *userdata) { _cleanup_strv_free_ char **names = NULL; _cleanup_free_ char *target = NULL; const char *verb = argv[0]; + UnitFileChange *changes = NULL; + unsigned n_changes = 0; UnitDependency dep; int r = 0; @@ -5672,13 +5674,8 @@ static int add_dependency(int argc, char *argv[], void *userdata) { assert_not_reached("Unknown verb"); if (install_client_side()) { - UnitFileChange *changes = NULL; - unsigned n_changes = 0; - r = unit_file_add_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, &changes, &n_changes); unit_file_dump_changes(r, "add dependency on", changes, n_changes, arg_quiet); - unit_file_changes_free(changes, n_changes); - return r; } else { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL, *m = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -5712,27 +5709,32 @@ static int add_dependency(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "Failed to add dependency: %s", bus_error_message(&error, r)); - r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL); + r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, &changes, &n_changes); if (r < 0) - return r; + goto finish; - if (arg_no_reload) - return 0; - return daemon_reload(argc, argv, userdata); + if (arg_no_reload) { + r = 0; + goto finish; + } + + r = daemon_reload(argc, argv, userdata); } + +finish: + unit_file_changes_free(changes, n_changes); + + return r; } static int preset_all(int argc, char *argv[], void *userdata) { + UnitFileChange *changes = NULL; + unsigned n_changes = 0; int r; if (install_client_side()) { - UnitFileChange *changes = NULL; - unsigned n_changes = 0; - r = unit_file_preset_all(arg_scope, arg_runtime, arg_root, arg_preset_mode, arg_force, &changes, &n_changes); unit_file_dump_changes(r, "preset", changes, n_changes, arg_quiet); - unit_file_changes_free(changes, n_changes); - return r; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; @@ -5759,14 +5761,22 @@ static int preset_all(int argc, char *argv[], void *userdata) { if (r < 0) return log_error_errno(r, "Failed to preset all units: %s", bus_error_message(&error, r)); - r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, NULL, NULL); + r = bus_deserialize_and_dump_unit_file_changes(reply, arg_quiet, &changes, &n_changes); if (r < 0) - return r; + goto finish; - if (arg_no_reload) - return 0; - return daemon_reload(argc, argv, userdata); + if (arg_no_reload) { + r = 0; + goto finish; + } + + r = daemon_reload(argc, argv, userdata); } + +finish: + unit_file_changes_free(changes, n_changes); + + return r; } static int unit_is_enabled(int argc, char *argv[], void *userdata) { commit 5f056378b0ceffb6e6fba3513f7eae72e2d09dc8 Author: Christian Hesse Date: Fri May 27 09:32:41 2016 +0200 systemctl: fix return values on success diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c index 94c5901..f4cdfa9 100644 --- a/src/systemctl/systemctl.c +++ b/src/systemctl/systemctl.c @@ -1439,6 +1439,8 @@ static int list_unit_files(int argc, char *argv[], void *userdata) { assert(c <= n_units); hashmap_free(h); + + r = 0; } else { _cleanup_(sd_bus_message_unrefp) sd_bus_message *m = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -2025,6 +2027,7 @@ static int get_default(int argc, char *argv[], void *userdata) { return log_error_errno(r, "Failed to get default target: %m"); path = _path; + r = 0; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; sd_bus *bus; @@ -2072,6 +2075,9 @@ static int set_default(int argc, char *argv[], void *userdata) { if (install_client_side()) { r = unit_file_set_default(arg_scope, arg_root, unit, true, &changes, &n_changes); unit_file_dump_changes(r, "set default", changes, n_changes, arg_quiet); + + if (r > 0) + r = 0; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; @@ -5676,6 +5682,9 @@ static int add_dependency(int argc, char *argv[], void *userdata) { if (install_client_side()) { r = unit_file_add_dependency(arg_scope, arg_runtime, arg_root, names, target, dep, arg_force, &changes, &n_changes); unit_file_dump_changes(r, "add dependency on", changes, n_changes, arg_quiet); + + if (r > 0) + r = 0; } else { _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL, *m = NULL; _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; @@ -5735,6 +5744,9 @@ static int preset_all(int argc, char *argv[], void *userdata) { if (install_client_side()) { r = unit_file_preset_all(arg_scope, arg_runtime, arg_root, arg_preset_mode, arg_force, &changes, &n_changes); unit_file_dump_changes(r, "preset", changes, n_changes, arg_quiet); + + if (r > 0) + r = 0; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; @@ -5817,6 +5829,7 @@ static int unit_is_enabled(int argc, char *argv[], void *userdata) { puts(unit_file_state_to_string(state)); } + r = 0; } else { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; sd_bus *bus;