Consider nosound and novideo when keeping groups & misc refactors

[kmk3]

Even when nogroups is not used, avoid keeping the audio and video
groups when nosound and novideo are used, respectively.

Based on @rusty-snake's suggestion:
#4603 (comment)

Relates to #4603.

---

Note: I haven't tested this.

[rusty-snake]

As noted in #4603 (comment), dropping video may break hw-accel. Otherwise this sounds reasonable.

[kmk3]

From https://github.com/netblue30/firejail/runs/3979333974?check_suite_focus=true:

gcc-11 -g -O2 -ggdb -W -Wall -Werror -O2 -DVERSION='"0.9.67"'  -DPREFIX='"/usr"' -DSYSCONFDIR='"/etc/firejail"' -DLIBDIR='"/usr/lib"' -DBINDIR='"/usr/bin"'   -DVARDIR='"/var/lib/firejail"'  -DHAVE_OUTPUT -DHAVE_X11 -DHAVE_PRIVATE_HOME -DHAVE_APPARMOR  -DHAVE_USERTMPFS -DHAVE_DBUSPROXY -DHAVE_FIRETUNNEL -DHAVE_GLOBALCFG -DHAVE_CHROOT -DHAVE_NETWORK -DHAVE_USERNS -DHAVE_FILE_TRANSFER -DHAVE_SELINUX -DHAVE_SUID  -fstack-protector-all -D_FORTIFY_SOURCE=2 -fPIE -Wformat -Wformat-security -fstack-clash-protection -fstack-protector-strong -fanalyzer -Wno-analyzer-malloc-leak   -c util.c -o util.o
util.c: In function ‘copy_group_ifcont’:
util.c:129:17: error: value computed is not used [-Werror=unused-value]
  129 |                 *new_ngroups++;
      |                 ^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:7: util.o] Error 1
make: *** [Makefile:42: src/firejail/firejail] Error 2
make[1]: Leaving directory '/home/runner/work/firejail/firejail/src/firejail'
Error: Process completed with exit code 2.

Force-pushed to fix the above.

@rusty-snake commented on Oct 22:

As noted in #4603
(comment),
dropping video may break hw-accel. Otherwise this sounds reasonable.

if !arg_no3d: // /dev/dri/card0 has group video
    drop_group("render")

I don't use nvidia and that path is also owned by the "video" group on my
system. I do have /dev/dri/render* owned by the "render" group though.

And this PR only changes the handling of the "video" group, not the "render"
group.

But from #3644, I now see that this could be an issue on e.g.: /dev/nvidia*.

I'll add some more checks for that.

[kmk3]

I'll add some more checks for that.

I think I got it.

@rusty-snake

But are you sure that novideo by itself breaks hw-accel on nvidia? Or does
that only happen with nogroups?

After re-reading #3644 and the related code, if firejail detects nvidia, then
what happens is that nogroups is ignored, which prevents not only the "video"
group, but also the "render" group from being dropped. It could be that either
both are needed or that just the "render" group is needed for hw-accel.

Cc: @hlein (from #3644)

[rusty-snake]

novideo is only about webcams.

About nVidia IDK nothing (except that the driver situation is a shit). I've some intel graphics

$ ls -l /dev/dri
insgesamt 0
drwxr-xr-x. 2 root root         80 18. Okt 08:24 by-path/
crw-rw----+ 1 root video  226,   0 18. Okt 08:24 card0
crw-rw-rw-. 1 root render 226, 128 18. Okt 08:24 renderD128
$ ls -l /sys/class/drm
insgesamt 0
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0-eDP-1 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-eDP-1/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0-HDMI-A-1 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-HDMI-A-1/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 renderD128 -> ../../devices/pci0000:00/0000:00:02.0/drm/renderD128/
-r--r--r--. 1 root root 4,0K 21. Okt 22:17 version

As you can see renderD128 and card0 are both the same PCIe device. IDK what the differences is or for what they are used. Maybe card0: OpenGL/Vulkan, renderD128: VAAPI?

[kmk3]

@rusty-snake commented on Oct 22:

novideo is only about webcams.

Then why did you say that "dropping video may break hw-accel"?

I mentioned novideo because e.g.: /dev/dri/card0 is owned by root:video
(and devices owned by root:video may or may not be needed for nvidia hw-accel),
and this PR is partly about mapping the video group to novideo (similarly to
your code suggestion).

Relevant code:

• firejail/src/firejail/fs_dev.c

  Lines 34 to 140 in 44c15b6

  	// device type
  	typedef enum {
  	DEV_NONE = 0,
  	DEV_SOUND,
  	DEV_3D,
  	DEV_VIDEO,
  	DEV_TV,
  	DEV_DVD,
  	DEV_U2F,
  	DEV_INPUT
  	} DEV_TYPE;
  	typedef struct {
  	const char *dev_fname;
  	const char *run_fname;
  	DEV_TYPE type;
  	} DevEntry;
  	static DevEntry dev[] = {
  	{"/dev/snd", RUN_DEV_DIR "/snd", DEV_SOUND}, // sound device
  	{"/dev/dri", RUN_DEV_DIR "/dri", DEV_3D}, // 3d device
  	{"/dev/nvidia0", RUN_DEV_DIR "/nvidia0", DEV_3D},
  	{"/dev/nvidia1", RUN_DEV_DIR "/nvidia1", DEV_3D},
  	{"/dev/nvidia2", RUN_DEV_DIR "/nvidia2", DEV_3D},
  	{"/dev/nvidia3", RUN_DEV_DIR "/nvidia3", DEV_3D},
  	{"/dev/nvidia4", RUN_DEV_DIR "/nvidia4", DEV_3D},
  	{"/dev/nvidia5", RUN_DEV_DIR "/nvidia5", DEV_3D},
  	{"/dev/nvidia6", RUN_DEV_DIR "/nvidia6", DEV_3D},
  	{"/dev/nvidia7", RUN_DEV_DIR "/nvidia7", DEV_3D},
  	{"/dev/nvidia8", RUN_DEV_DIR "/nvidia8", DEV_3D},
  	{"/dev/nvidia9", RUN_DEV_DIR "/nvidia9", DEV_3D},
  	{"/dev/nvidiactl", RUN_DEV_DIR "/nvidiactl", DEV_3D},
  	{"/dev/nvidia-modeset", RUN_DEV_DIR "/nvidia-modeset", DEV_3D},
  	{"/dev/nvidia-uvm", RUN_DEV_DIR "/nvidia-uvm", DEV_3D},
  	{"/dev/video0", RUN_DEV_DIR "/video0", DEV_VIDEO}, // video camera devices
  	{"/dev/video1", RUN_DEV_DIR "/video1", DEV_VIDEO},
  	{"/dev/video2", RUN_DEV_DIR "/video2", DEV_VIDEO},
  	{"/dev/video3", RUN_DEV_DIR "/video3", DEV_VIDEO},
  	{"/dev/video4", RUN_DEV_DIR "/video4", DEV_VIDEO},
  	{"/dev/video5", RUN_DEV_DIR "/video5", DEV_VIDEO},
  	{"/dev/video6", RUN_DEV_DIR "/video6", DEV_VIDEO},
  	{"/dev/video7", RUN_DEV_DIR "/video7", DEV_VIDEO},
  	{"/dev/video8", RUN_DEV_DIR "/video8", DEV_VIDEO},
  	{"/dev/video9", RUN_DEV_DIR "/video9", DEV_VIDEO},
  	{"/dev/dvb", RUN_DEV_DIR "/dvb", DEV_TV}, // DVB (Digital Video Broadcasting) - TV device
  	{"/dev/sr0", RUN_DEV_DIR "/sr0", DEV_DVD}, // for DVD and audio CD players
  	{"/dev/hidraw0", RUN_DEV_DIR "/hidraw0", DEV_U2F},
  	{"/dev/hidraw1", RUN_DEV_DIR "/hidraw1", DEV_U2F},
  	{"/dev/hidraw2", RUN_DEV_DIR "/hidraw2", DEV_U2F},
  	{"/dev/hidraw3", RUN_DEV_DIR "/hidraw3", DEV_U2F},
  	{"/dev/hidraw4", RUN_DEV_DIR "/hidraw4", DEV_U2F},
  	{"/dev/hidraw5", RUN_DEV_DIR "/hidraw5", DEV_U2F},
  	{"/dev/hidraw6", RUN_DEV_DIR "/hidraw6", DEV_U2F},
  	{"/dev/hidraw7", RUN_DEV_DIR "/hidraw7", DEV_U2F},
  	{"/dev/hidraw8", RUN_DEV_DIR "/hidraw8", DEV_U2F},
  	{"/dev/hidraw9", RUN_DEV_DIR "/hidraw9", DEV_U2F},
  	{"/dev/usb", RUN_DEV_DIR "/usb", DEV_U2F}, // USB devices such as Yubikey, U2F
  	{"/dev/input", RUN_DEV_DIR "/input", DEV_INPUT},
  	{NULL, NULL, DEV_NONE}
  	};
  	static void deventry_mount(void) {
  	int i = 0;
  	while (dev[i].dev_fname != NULL) {
  	struct stat s;
  	if (stat(dev[i].run_fname, &s) == 0) {
  	// check device type and subsystem configuration
  	if ((dev[i].type == DEV_SOUND && arg_nosound == 0) ||
  	(dev[i].type == DEV_3D && arg_no3d == 0) ||
  	(dev[i].type == DEV_VIDEO && arg_novideo == 0) ||
  	(dev[i].type == DEV_TV && arg_notv == 0) ||
  	(dev[i].type == DEV_DVD && arg_nodvd == 0) ||
  	(dev[i].type == DEV_U2F && arg_nou2f == 0) ||
  	(dev[i].type == DEV_INPUT && arg_noinput == 0)) {
  	int dir = is_dir(dev[i].run_fname);
  	if (arg_debug)
  	printf("mounting %s %s\n", dev[i].run_fname, (dir)? "directory": "file");
  	if (dir) {
  	mkdir_attr(dev[i].dev_fname, 0755, 0, 0);
  	}
  	else {
  	struct stat s;
  	if (stat(dev[i].run_fname, &s) == -1) {
  	if (arg_debug)
  	fwarning("cannot stat %s file\n", dev[i].run_fname);
  	i++;
  	continue;
  	}
  	FILE *fp = fopen(dev[i].dev_fname, "we");
  	if (fp) {
  	fprintf(fp, "\n");
  	SET_PERMS_STREAM(fp, s.st_uid, s.st_gid, s.st_mode);
  	fclose(fp);
  	}
  	}
  	if (mount(dev[i].run_fname, dev[i].dev_fname, NULL, MS_BIND|MS_REC, NULL) < 0)
  	errExit("mounting dev file");
  	fs_logger2("whitelist", dev[i].dev_fname);
  	}
  	}
  	i++;
  	}
  	}

• firejail/src/firejail/fs_dev.c

  Lines 351 to 358 in 44c15b6

  	void fs_dev_disable_video(void) {
  	int i = 0;
  	while (dev[i].dev_fname != NULL) {
  	if (dev[i].type == DEV_VIDEO)
  	disable_file_or_dir(dev[i].dev_fname);
  	i++;
  	}
  	}

Misc: This PR is basically part 1 of trying to fix #4603 (i.e.: that nogroups
breaks audio depending on the system). I'll add some more comments in there
later.

About nVidia IDK nothing (except that the driver situation is a shit). I've
some intel graphics

$ ls -l /dev/dri
insgesamt 0
drwxr-xr-x. 2 root root         80 18. Okt 08:24 by-path/
crw-rw----+ 1 root video  226,   0 18. Okt 08:24 card0
crw-rw-rw-. 1 root render 226, 128 18. Okt 08:24 renderD128
$ ls -l /sys/class/drm
insgesamt 0
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0-eDP-1 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-eDP-1/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 card0-HDMI-A-1 -> ../../devices/pci0000:00/0000:00:02.0/drm/card0/card0-HDMI-A-1/
lrwxrwxrwx. 1 root root    0 21. Okt 22:17 renderD128 -> ../../devices/pci0000:00/0000:00:02.0/drm/renderD128/
-r--r--r--. 1 root root 4,0K 21. Okt 22:17 version

As you can see renderD128 and card0 are both the same PCIe device.

It's basically the same thing in here (AMD).

IDK what the differences is or for what they are used. Maybe card0:
OpenGL/Vulkan, renderD128: VAAPI?

I don't know exactly what each device file does, but maybe they can be used for
raw video out capture or raw device access in general (which the nvidia driver
may or may not need).

[netblue30]

Fine with me.

[hlein]

I'll add some more checks for that.

I think I got it.

@rusty-snake

But are you sure that novideo by itself breaks hw-accel on nvidia? Or does that only happen with nogroups?

After re-reading #3644 and the related code, if firejail detects nvidia, then what happens is that nogroups is ignored, which prevents not only the "video" group, but also the "render" group from being dropped. It could be that either both are needed or that just the "render" group is needed for hw-accel.

Cc: @hlein (from #3644)

I don't have the whole thing reloaded into my brain, but I tried the reproducer from #3644 and variants:

$ firejail --version
firejail version 0.9.66

$ glxgears   ### Works
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

$ firejail --noprofile glxgears   ### Works
Parent pid 14201, child pid 14202
Warning: not remounting [redacted]
Child process initialized in 7.98 ms
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

$ firejail --noprofile --nogroups glxgears   ### Fails
Parent pid 13354, child pid 13355
Warning: not remounting [redacted]
Child process initialized in 8.00 ms
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  149 (GLX)
  Minor opcode of failed request:  3 (X_GLXCreateContext)
  Value in failed request:  0x0
  Serial number of failed request:  51
  Current serial number in output stream:  52

Parent is shutting down, bye...

$ firejail --noprofile --novideo glxgears   ### Works
Parent pid 13419, child pid 13420
Warning: not remounting [redacted]
Child process initialized in 7.85 ms
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

So: notwithstanding any auto-detection-induced behavior changes that firejail might be doing, nogroups is enough to cause failure, novideo works.

[kmk3]

@hlein commented on Nov 8:

I'll add some more checks for that.

I think I got it.

@rusty-snake

But are you sure that novideo by itself breaks hw-accel on nvidia? Or
does that only happen with nogroups?

After re-reading #3644 and the related code, if firejail detects nvidia,
then what happens is that nogroups is ignored, which prevents not only
the "video" group, but also the "render" group from being dropped. It could
be that either both are needed or that just the "render" group is needed
for hw-accel.

Cc: @hlein (from #3644)

I don't have the whole thing reloaded into my brain, but I tried the
reproducer from #3644 and variants:

$ firejail --version
firejail version 0.9.66

$ glxgears   ### Works
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

$ firejail --noprofile glxgears   ### Works
Parent pid 14201, child pid 14202
Warning: not remounting [redacted]
Child process initialized in 7.98 ms
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

$ firejail --noprofile --nogroups glxgears   ### Fails
Parent pid 13354, child pid 13355
Warning: not remounting [redacted]
Child process initialized in 8.00 ms
X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  149 (GLX)
  Minor opcode of failed request:  3 (X_GLXCreateContext)
  Value in failed request:  0x0
  Serial number of failed request:  51
  Current serial number in output stream:  52

Parent is shutting down, bye...

$ firejail --noprofile --novideo glxgears   ### Works
Parent pid 13419, child pid 13420
Warning: not remounting [redacted]
Child process initialized in 7.85 ms
Running synchronized to the vertical refresh.  The framerate should be
approximately the same as the monitor refresh rate.
...

So: notwithstanding any auto-detection-induced behavior changes that firejail
might be doing, nogroups is enough to cause failure, novideo works.

Hello, thanks for re-testing!

Considering the above, it seems that unless novideo misses a nvidia device
that is both required for hw-accel and that is also owned by the "video" group
(rather than the usual "render" group) for some reason, then novideo
shouldn't affect hw-accel (with or without these changes).

The udev rules don't seem to do anything special for nvidia GPUs either:

$ pacman -Q udev
eudev 249.5-1
$ grep '"video"' /usr/lib/udev/rules.d/*
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="video4linux", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="graphics", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="drm", KERNEL!="renderD*", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="dvb", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="media", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="cec", GROUP="video"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="firewire", TEST=="units", ENV{IEEE1394_UNIT_FUNCTION_VIDEO}=="1", GROUP="video"
$ grep '"render"' /usr/lib/udev/rules.d/*
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="drm", KERNEL=="renderD*", GROUP="render", MODE="0666"
/usr/lib/udev/rules.d/50-udev-default.rules:SUBSYSTEM=="kfd", GROUP="render", MODE="0666"
$ grep -i nvidia /usr/lib/udev/rules.d/*
/usr/lib/udev/rules.d/51-android.rules:#        Nvidia
/usr/lib/udev/rules.d/51-android.rules:ATTR{idVendor}!="0955", GOTO="not_Nvidia"
/usr/lib/udev/rules.d/51-android.rules:LABEL="not_Nvidia"
/usr/lib/udev/rules.d/70-steam-input.rules:# NVIDIA Shield Portable (2013 - NVIDIA_Controller_v01.01 - In-Home Streaming only)
/usr/lib/udev/rules.d/70-steam-input.rules:# NVIDIA Shield Controller (2015 - NVIDIA_Controller_v01.03 over USB hidraw)
/usr/lib/udev/rules.d/70-steam-input.rules:# NVIDIA Shield Controller (2017 - NVIDIA_Controller_v01.04 over bluetooth hidraw)

@rusty-snake What do you think?

[rusty-snake]

I still don't understand what all this has to do with nVidia.

[kmk3]

@rusty-snake commented on Nov 9:

I still don't understand what all this has to do with nVidia.

It's because of this code on src/firejail/profile.c:

	else if (strcmp(ptr, "nogroups") == 0) {
		// nvidia cards require video group; disable nogroups
		if (access("/dev/nvidiactl", R_OK) == 0 && arg_no3d == 0) {
			fwarning("Warning: NVIDIA card detected, nogroups command disabled\n");
			arg_nogroups = 0;
		}
		else
			arg_nogroups = 1;
		return 0;
	}

It's a workaround for nvidia that mentions "video group" and checks for
arg_no3d.

(As the one making the changes, in my mind it seemed obvious, but I should
probably have referenced it sooner)

See the following commits:

• 623e682 ("temporary fix for nvidia/nogroups/noroot issue (#3644, #841)",
  2020-10-02)
• cb460c3 ("more nvidia (#3644)", 2020-10-03)

You even suggested the latter one apparently:

• nvidia: opengl errors with nvidia proprietary driver due to "nogroups" #3644 (comment)

Related issues:

• nvidia driver and noroot setting #841
• nvidia: opengl errors with nvidia proprietary driver due to "nogroups" #3644

On the latter issue, it appears that either the "video" or "render" group (or
both) were needed to enable hw-accel of OpenGL on Chromium on Gentoo.

After reading some more of #3644, it looks like this PR is probably the
definitive fix for it coincidentally, so we could probably drop the workaround
on profile.c after this PR (provided that it wouldn't cause issues considering
the "drop all groups" instances in other parts of the code), but I'd rather do
that in a new PR.

[kmk3]

@kmk3 commented on Nov 10:

After reading some more of #3644, it looks like this PR is probably the
definitive fix for it coincidentally, so we could probably drop the
workaround on profile.c after this PR (provided that it wouldn't cause issues
considering the "drop all groups" instances in other parts of the code), but
I'd rather do that in a new PR.

Sorry, I was getting ahead of myself. I mean drop the workaround once we split
the handling of these groups (i.e.: the ones mapped to no options) from the
handling of nogroups (and maybe also from the handling of noroot) on
src/firejail/main.c (WIP). Then each of these groups would always be kept
(unless e.g.: nosound is enabled), even when nogroups is enabled.

I keep forgetting that this PR only does organizing and some hardening and that
the WIP changes are the ones intended to increase compatibility.

[smitsohu]

Merged!