* [PATCH v2] nbd/server: Add --selinux-label option @ 2021-07-23 10:33 Richard W.M. Jones 2021-07-23 10:33 ` Richard W.M. Jones 0 siblings, 1 reply; 13+ messages in thread From: Richard W.M. Jones @ 2021-07-23 10:33 UTC (permalink / raw) To: eblake; +Cc: vsementsov, berrange, qemu-devel, qemu-block v1 was here: https://lists.nongnu.org/archive/html/qemu-block/2021-07/threads.html#00713 v2 adds the changes to CI docker files as suggested by Dan Berrange in his review. Rich. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 10:33 [PATCH v2] nbd/server: Add --selinux-label option Richard W.M. Jones @ 2021-07-23 10:33 ` Richard W.M. Jones 2021-07-23 10:47 ` Daniel P. Berrangé ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Richard W.M. Jones @ 2021-07-23 10:33 UTC (permalink / raw) To: eblake; +Cc: vsementsov, berrange, qemu-devel, qemu-block Under SELinux, Unix domain sockets have two labels. One is on the disk and can be set with commands such as chcon(1). There is a different label stored in memory (called the process label). This can only be set by the process creating the socket. When using SELinux + SVirt and wanting qemu to be able to connect to a qemu-nbd instance, you must set both labels correctly first. For qemu-nbd the options to set the second label are awkward. You can create the socket in a wrapper program and then exec into qemu-nbd. Or you could try something with LD_PRELOAD. This commit adds the ability to set the label straightforwardly on the command line, via the new --selinux-label flag. (The name of the flag is the same as the equivalent nbdkit option.) A worked example showing how to use the new option can be found in this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 Signed-off-by: Richard W.M. Jones <rjones@redhat.com> --- configure | 9 ++++- meson.build | 10 +++++- meson_options.txt | 3 ++ qemu-nbd.c | 33 +++++++++++++++++++ tests/docker/dockerfiles/centos8.docker | 1 + tests/docker/dockerfiles/fedora.docker | 1 + tests/docker/dockerfiles/opensuse-leap.docker | 1 + tests/docker/dockerfiles/ubuntu1804.docker | 1 + tests/docker/dockerfiles/ubuntu2004.docker | 1 + 9 files changed, 58 insertions(+), 2 deletions(-) diff --git a/configure b/configure index b5965b159f..7e04bd485f 100755 --- a/configure +++ b/configure @@ -443,6 +443,7 @@ fuse="auto" fuse_lseek="auto" multiprocess="auto" slirp_smbd="$default_feature" +selinux="auto" malloc_trim="auto" gio="$default_feature" @@ -1578,6 +1579,10 @@ for opt do ;; --disable-slirp-smbd) slirp_smbd=no ;; + --enable-selinux) selinux="enabled" + ;; + --disable-selinux) selinux="disabled" + ;; *) echo "ERROR: unknown option $opt" echo "Try '$0 --help' for more information" @@ -1965,6 +1970,7 @@ disabled with --disable-FEATURE, default is enabled if available multiprocess Out of process device emulation support gio libgio support slirp-smbd use smbd (at path --smbd=*) in slirp networking + selinux SELinux support in qemu-nbd NOTE: The object files are built at the place where configure is launched EOF @@ -5220,7 +5226,8 @@ if test "$skip_meson" = no; then -Dattr=$attr -Ddefault_devices=$default_devices -Dvirglrenderer=$virglrenderer \ -Ddocs=$docs -Dsphinx_build=$sphinx_build -Dinstall_blobs=$blobs \ -Dvhost_user_blk_server=$vhost_user_blk_server -Dmultiprocess=$multiprocess \ - -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf\ + -Dfuse=$fuse -Dfuse_lseek=$fuse_lseek -Dguest_agent_msi=$guest_agent_msi -Dbpf=$bpf \ + -Dselinux=$selinux \ $(if test "$default_features" = no; then echo "-Dauto_features=disabled"; fi) \ -Dtcg_interpreter=$tcg_interpreter \ $cross_arg \ diff --git a/meson.build b/meson.build index 2f377098d7..2d7206233e 100644 --- a/meson.build +++ b/meson.build @@ -1064,6 +1064,11 @@ keyutils = dependency('libkeyutils', required: false, has_gettid = cc.has_function('gettid') +# libselinux +selinux = dependency('libselinux', + required: get_option('selinux'), + method: 'pkg-config', kwargs: static_kwargs) + # Malloc tests malloc = [] @@ -1291,6 +1296,7 @@ config_host_data.set('CONFIG_FUSE', fuse.found()) config_host_data.set('CONFIG_FUSE_LSEEK', fuse_lseek.found()) config_host_data.set('CONFIG_X11', x11.found()) config_host_data.set('CONFIG_CFI', get_option('cfi')) +config_host_data.set('CONFIG_SELINUX', selinux.found()) config_host_data.set('QEMU_VERSION', '"@0@"'.format(meson.project_version())) config_host_data.set('QEMU_VERSION_MAJOR', meson.project_version().split('.')[0]) config_host_data.set('QEMU_VERSION_MINOR', meson.project_version().split('.')[1]) @@ -2739,7 +2745,8 @@ if have_tools qemu_io = executable('qemu-io', files('qemu-io.c'), dependencies: [block, qemuutil], install: true) qemu_nbd = executable('qemu-nbd', files('qemu-nbd.c'), - dependencies: [blockdev, qemuutil, gnutls], install: true) + dependencies: [blockdev, qemuutil, gnutls, selinux], + install: true) subdir('storage-daemon') subdir('contrib/rdmacm-mux') @@ -3104,6 +3111,7 @@ summary_info += {'libpmem support': libpmem.found()} summary_info += {'libdaxctl support': libdaxctl.found()} summary_info += {'libudev': libudev.found()} summary_info += {'FUSE lseek': fuse_lseek.found()} +summary_info += {'selinux': selinux.found()} summary(summary_info, bool_yn: true, section: 'Dependencies') if not supported_cpus.contains(cpu) diff --git a/meson_options.txt b/meson_options.txt index a9a9b8f4c6..a5938500a3 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -155,3 +155,6 @@ option('slirp', type: 'combo', value: 'auto', option('fdt', type: 'combo', value: 'auto', choices: ['disabled', 'enabled', 'auto', 'system', 'internal'], description: 'Whether and how to find the libfdt library') + +option('selinux', type: 'feature', value: 'auto', + description: 'SELinux support in qemu-nbd') diff --git a/qemu-nbd.c b/qemu-nbd.c index 26ffbf15af..003ba2492e 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -47,6 +47,10 @@ #include "trace/control.h" #include "qemu-version.h" +#ifdef CONFIG_SELINUX +#include <selinux/selinux.h> +#endif + #ifdef __linux__ #define HAVE_NBD_DEVICE 1 #else @@ -64,6 +68,7 @@ #define QEMU_NBD_OPT_FORK 263 #define QEMU_NBD_OPT_TLSAUTHZ 264 #define QEMU_NBD_OPT_PID_FILE 265 +#define QEMU_NBD_OPT_SELINUX_LABEL 266 #define MBR_SIZE 512 @@ -116,6 +121,9 @@ static void usage(const char *name) " --fork fork off the server process and exit the parent\n" " once the server is running\n" " --pid-file=PATH store the server's process ID in the given file\n" +#ifdef CONFIG_SELINUX +" --selinux-label=LABEL set SELinux process label on listening socket\n" +#endif #if HAVE_NBD_DEVICE "\n" "Kernel NBD client support:\n" @@ -532,6 +540,8 @@ int main(int argc, char **argv) { "trace", required_argument, NULL, 'T' }, { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE }, + { "selinux-label", required_argument, NULL, + QEMU_NBD_OPT_SELINUX_LABEL }, { NULL, 0, NULL, 0 } }; int ch; @@ -558,6 +568,7 @@ int main(int argc, char **argv) int old_stderr = -1; unsigned socket_activation; const char *pid_file_name = NULL; + const char *selinux_label = NULL; BlockExportOptions *export_opts; #ifdef CONFIG_POSIX @@ -747,6 +758,9 @@ int main(int argc, char **argv) case QEMU_NBD_OPT_PID_FILE: pid_file_name = optarg; break; + case QEMU_NBD_OPT_SELINUX_LABEL: + selinux_label = optarg; + break; } } @@ -938,6 +952,16 @@ int main(int argc, char **argv) } else { backlog = MIN(shared, SOMAXCONN); } + if (sockpath && selinux_label) { +#ifdef CONFIG_SELINUX + if (setsockcreatecon_raw(selinux_label) == -1) { + error_report("Cannot set SELinux socket create context " + "to %s: %s", + selinux_label, strerror(errno)); + exit(EXIT_FAILURE); + } +#endif + } saddr = nbd_build_socket_address(sockpath, bindto, port); if (qio_net_listener_open_sync(server, saddr, backlog, &local_err) < 0) { @@ -945,6 +969,15 @@ int main(int argc, char **argv) error_report_err(local_err); exit(EXIT_FAILURE); } + if (sockpath && selinux_label) { +#ifdef CONFIG_SELINUX + if (setsockcreatecon_raw(NULL) == -1) { + error_report("Cannot clear SELinux socket create context: %s", + strerror(errno)); + exit(EXIT_FAILURE); + } +#endif + } } else { size_t i; /* See comment in check_socket_activation above. */ diff --git a/tests/docker/dockerfiles/centos8.docker b/tests/docker/dockerfiles/centos8.docker index 46398c61ee..7f135f8e8c 100644 --- a/tests/docker/dockerfiles/centos8.docker +++ b/tests/docker/dockerfiles/centos8.docker @@ -51,6 +51,7 @@ ENV PACKAGES \ libpng-devel \ librbd-devel \ libseccomp-devel \ + libselinux-devel \ libslirp-devel \ libssh-devel \ libtasn1-devel \ diff --git a/tests/docker/dockerfiles/fedora.docker b/tests/docker/dockerfiles/fedora.docker index eec1add7f6..c6fd7e1113 100644 --- a/tests/docker/dockerfiles/fedora.docker +++ b/tests/docker/dockerfiles/fedora.docker @@ -53,6 +53,7 @@ ENV PACKAGES \ libpng-devel \ librbd-devel \ libseccomp-devel \ + libselinux-devel \ libslirp-devel \ libssh-devel \ libtasn1-devel \ diff --git a/tests/docker/dockerfiles/opensuse-leap.docker b/tests/docker/dockerfiles/opensuse-leap.docker index 5a8bee0289..3bbdb67f4f 100644 --- a/tests/docker/dockerfiles/opensuse-leap.docker +++ b/tests/docker/dockerfiles/opensuse-leap.docker @@ -55,6 +55,7 @@ ENV PACKAGES \ libpulse-devel \ librbd-devel \ libseccomp-devel \ + libselinux-devel \ libspice-server-devel \ libssh-devel \ libtasn1-devel \ diff --git a/tests/docker/dockerfiles/ubuntu1804.docker b/tests/docker/dockerfiles/ubuntu1804.docker index 0880bf3e29..450fd06d0d 100644 --- a/tests/docker/dockerfiles/ubuntu1804.docker +++ b/tests/docker/dockerfiles/ubuntu1804.docker @@ -60,6 +60,7 @@ ENV PACKAGES \ libsdl2-dev \ libsdl2-image-dev \ libseccomp-dev \ + libselinux-dev \ libsnappy-dev \ libspice-protocol-dev \ libspice-server-dev \ diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker index 39de63d012..15a026be09 100644 --- a/tests/docker/dockerfiles/ubuntu2004.docker +++ b/tests/docker/dockerfiles/ubuntu2004.docker @@ -60,6 +60,7 @@ ENV PACKAGES \ libsdl2-dev \ libsdl2-image-dev \ libseccomp-dev \ + libselinux-dev \ libslirp-dev \ libsnappy-dev \ libspice-protocol-dev \ -- 2.31.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 10:33 ` Richard W.M. Jones @ 2021-07-23 10:47 ` Daniel P. Berrangé 2021-07-26 14:22 ` Eric Blake 2021-07-23 16:18 ` Kevin Wolf 2021-09-27 21:18 ` Eric Blake 2 siblings, 1 reply; 13+ messages in thread From: Daniel P. Berrangé @ 2021-07-23 10:47 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: vsementsov, eblake, qemu-devel, qemu-block On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote: > Under SELinux, Unix domain sockets have two labels. One is on the > disk and can be set with commands such as chcon(1). There is a > different label stored in memory (called the process label). This can > only be set by the process creating the socket. When using SELinux + > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > you must set both labels correctly first. > > For qemu-nbd the options to set the second label are awkward. You can > create the socket in a wrapper program and then exec into qemu-nbd. > Or you could try something with LD_PRELOAD. > > This commit adds the ability to set the label straightforwardly on the > command line, via the new --selinux-label flag. (The name of the flag > is the same as the equivalent nbdkit option.) > > A worked example showing how to use the new option can be found in > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > --- > configure | 9 ++++- > meson.build | 10 +++++- > meson_options.txt | 3 ++ > qemu-nbd.c | 33 +++++++++++++++++++ > tests/docker/dockerfiles/centos8.docker | 1 + > tests/docker/dockerfiles/fedora.docker | 1 + > tests/docker/dockerfiles/opensuse-leap.docker | 1 + > tests/docker/dockerfiles/ubuntu1804.docker | 1 + > tests/docker/dockerfiles/ubuntu2004.docker | 1 + > 9 files changed, 58 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 10:47 ` Daniel P. Berrangé @ 2021-07-26 14:22 ` Eric Blake 0 siblings, 0 replies; 13+ messages in thread From: Eric Blake @ 2021-07-26 14:22 UTC (permalink / raw) To: Daniel P. Berrangé Cc: vsementsov, Richard W.M. Jones, qemu-block, qemu-devel On Fri, Jul 23, 2021 at 11:47:51AM +0100, Daniel P. Berrangé wrote: > On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > --- > > configure | 9 ++++- > > meson.build | 10 +++++- > > meson_options.txt | 3 ++ > > qemu-nbd.c | 33 +++++++++++++++++++ > > tests/docker/dockerfiles/centos8.docker | 1 + > > tests/docker/dockerfiles/fedora.docker | 1 + > > tests/docker/dockerfiles/opensuse-leap.docker | 1 + > > tests/docker/dockerfiles/ubuntu1804.docker | 1 + > > tests/docker/dockerfiles/ubuntu2004.docker | 1 + > > 9 files changed, 58 insertions(+), 2 deletions(-) > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Thanks. This is a new feature, so it doesn't qualify for inclusion in 6.1, but I'm queuing it through my NBD tree to go in as soon as upstream reopens for 6.2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 10:33 ` Richard W.M. Jones 2021-07-23 10:47 ` Daniel P. Berrangé @ 2021-07-23 16:18 ` Kevin Wolf 2021-07-23 16:34 ` Richard W.M. Jones 2021-07-23 16:38 ` Daniel P. Berrangé 2021-09-27 21:18 ` Eric Blake 2 siblings, 2 replies; 13+ messages in thread From: Kevin Wolf @ 2021-07-23 16:18 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: berrange, vsementsov, eblake, qemu-devel, qemu-block Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > Under SELinux, Unix domain sockets have two labels. One is on the > disk and can be set with commands such as chcon(1). There is a > different label stored in memory (called the process label). This can > only be set by the process creating the socket. When using SELinux + > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > you must set both labels correctly first. > > For qemu-nbd the options to set the second label are awkward. You can > create the socket in a wrapper program and then exec into qemu-nbd. > Or you could try something with LD_PRELOAD. > > This commit adds the ability to set the label straightforwardly on the > command line, via the new --selinux-label flag. (The name of the flag > is the same as the equivalent nbdkit option.) > > A worked example showing how to use the new option can be found in > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> I suppose this would also be relevant for the built-in NBD server, especially in the context of qemu-storage-daemon? If so, is this something specific to NBD sockets, or would it actually make sense to have it as a generic option in UnixSocketAddress? Kevin ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 16:18 ` Kevin Wolf @ 2021-07-23 16:34 ` Richard W.M. Jones 2021-07-23 16:38 ` Daniel P. Berrangé 1 sibling, 0 replies; 13+ messages in thread From: Richard W.M. Jones @ 2021-07-23 16:34 UTC (permalink / raw) To: Kevin Wolf; +Cc: berrange, vsementsov, eblake, qemu-devel, qemu-block On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > I suppose this would also be relevant for the built-in NBD server, > especially in the context of qemu-storage-daemon? > > If so, is this something specific to NBD sockets, or would it actually > make sense to have it as a generic option in UnixSocketAddress? For other NBD sockets, most likely. I'm not sure about Unix sockets in general (as in: I know they also have the two label thing, but I don't know if there's a situation where SVirt protects other sockets apart from NBD sockets). I'm sure Dan will know ... By the way although it appears that setsockcreatecon_raw is setting a global flag, it seems to actually use a thread-local variable, so implementing this (although still ugly) would not require locks. https://github.com/SELinuxProject/selinux/blob/32611aea6543e3a8f32635857e37b4332b0b5c99/libselinux/src/procattr.c#L347 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 16:18 ` Kevin Wolf 2021-07-23 16:34 ` Richard W.M. Jones @ 2021-07-23 16:38 ` Daniel P. Berrangé 2021-08-25 19:35 ` Eric Blake 1 sibling, 1 reply; 13+ messages in thread From: Daniel P. Berrangé @ 2021-07-23 16:38 UTC (permalink / raw) To: Kevin Wolf; +Cc: vsementsov, eblake, Richard W.M. Jones, qemu-block, qemu-devel On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > I suppose this would also be relevant for the built-in NBD server, > especially in the context of qemu-storage-daemon? It depends on the usage scenario really. nbdkit / qemu-nbd are not commonly run under any SELinux policy, so then end up being unconfined_t. A QEMU NBD client can't connect to an unconfined_t socket, so we need to override it with this arg. In the case of qemu system emulator, under libvirt, it will already have a svirt_t type, so in that case there is no need to override the type for the socket. For qsd there's not really any strong practice established but i expect most current usage is unconfined_t too and would benefit from setting label. > If so, is this something specific to NBD sockets, or would it actually > make sense to have it as a generic option in UnixSocketAddress? It is applicable to inet sockets too in fact. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 16:38 ` Daniel P. Berrangé @ 2021-08-25 19:35 ` Eric Blake 2021-09-24 19:23 ` Eric Blake 2021-09-27 12:55 ` Daniel P. Berrangé 0 siblings, 2 replies; 13+ messages in thread From: Eric Blake @ 2021-08-25 19:35 UTC (permalink / raw) To: Daniel P. Berrangé Cc: Kevin Wolf, vsementsov, Richard W.M. Jones, qemu-block, qemu-devel On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote: > On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: > > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > > > Under SELinux, Unix domain sockets have two labels. One is on the > > > disk and can be set with commands such as chcon(1). There is a > > > different label stored in memory (called the process label). This can > > > only be set by the process creating the socket. When using SELinux + > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > > you must set both labels correctly first. > > > > > > For qemu-nbd the options to set the second label are awkward. You can > > > create the socket in a wrapper program and then exec into qemu-nbd. > > > Or you could try something with LD_PRELOAD. > > > > > > This commit adds the ability to set the label straightforwardly on the > > > command line, via the new --selinux-label flag. (The name of the flag > > > is the same as the equivalent nbdkit option.) > > > > > > A worked example showing how to use the new option can be found in > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > > > I suppose this would also be relevant for the built-in NBD server, > > especially in the context of qemu-storage-daemon? > > It depends on the usage scenario really. nbdkit / qemu-nbd are > not commonly run under any SELinux policy, so then end up being > unconfined_t. A QEMU NBD client can't connect to an unconfined_t > socket, so we need to override it with this arg. > > In the case of qemu system emulator, under libvirt, it will > already have a svirt_t type, so in that case there is no need > to override the type for the socket. > > For qsd there's not really any strong practice established > but i expect most current usage is unconfined_t too and > would benefit from setting label. > > > If so, is this something specific to NBD sockets, or would it actually > > make sense to have it as a generic option in UnixSocketAddress? > > It is applicable to inet sockets too in fact. So now that 6.2 is open, should I queue the patch as is, or wait for a v3 that makes the option more generic to all socket usage? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-08-25 19:35 ` Eric Blake @ 2021-09-24 19:23 ` Eric Blake 2021-09-27 12:48 ` Vladimir Sementsov-Ogievskiy 2021-09-27 12:55 ` Daniel P. Berrangé 1 sibling, 1 reply; 13+ messages in thread From: Eric Blake @ 2021-09-24 19:23 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: Kevin Wolf, vsementsov, qemu-devel, qemu-block Ping On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote: > On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote: > > On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: > > > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > > > > Under SELinux, Unix domain sockets have two labels. One is on the > > > > disk and can be set with commands such as chcon(1). There is a > > > > different label stored in memory (called the process label). This can > > > > only be set by the process creating the socket. When using SELinux + > > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > > > you must set both labels correctly first. > > > > > > > > For qemu-nbd the options to set the second label are awkward. You can > > > > create the socket in a wrapper program and then exec into qemu-nbd. > > > > Or you could try something with LD_PRELOAD. > > > > > > > > This commit adds the ability to set the label straightforwardly on the > > > > command line, via the new --selinux-label flag. (The name of the flag > > > > is the same as the equivalent nbdkit option.) > > > > > > > > A worked example showing how to use the new option can be found in > > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > > > > > I suppose this would also be relevant for the built-in NBD server, > > > especially in the context of qemu-storage-daemon? > > > > It depends on the usage scenario really. nbdkit / qemu-nbd are > > not commonly run under any SELinux policy, so then end up being > > unconfined_t. A QEMU NBD client can't connect to an unconfined_t > > socket, so we need to override it with this arg. > > > > In the case of qemu system emulator, under libvirt, it will > > already have a svirt_t type, so in that case there is no need > > to override the type for the socket. > > > > For qsd there's not really any strong practice established > > but i expect most current usage is unconfined_t too and > > would benefit from setting label. > > > > > If so, is this something specific to NBD sockets, or would it actually > > > make sense to have it as a generic option in UnixSocketAddress? > > > > It is applicable to inet sockets too in fact. > > So now that 6.2 is open, should I queue the patch as is, or wait for a > v3 that makes the option more generic to all socket usage? > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-09-24 19:23 ` Eric Blake @ 2021-09-27 12:48 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 13+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2021-09-27 12:48 UTC (permalink / raw) To: Eric Blake, Daniel P. Berrangé; +Cc: Kevin Wolf, qemu-block, qemu-devel 24.09.2021 22:23, Eric Blake wrote: > Ping > > On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote: >> On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote: >>> On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: >>>> Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: >>>>> Under SELinux, Unix domain sockets have two labels. One is on the >>>>> disk and can be set with commands such as chcon(1). There is a >>>>> different label stored in memory (called the process label). This can >>>>> only be set by the process creating the socket. When using SELinux + >>>>> SVirt and wanting qemu to be able to connect to a qemu-nbd instance, >>>>> you must set both labels correctly first. >>>>> >>>>> For qemu-nbd the options to set the second label are awkward. You can >>>>> create the socket in a wrapper program and then exec into qemu-nbd. >>>>> Or you could try something with LD_PRELOAD. >>>>> >>>>> This commit adds the ability to set the label straightforwardly on the >>>>> command line, via the new --selinux-label flag. (The name of the flag >>>>> is the same as the equivalent nbdkit option.) >>>>> >>>>> A worked example showing how to use the new option can be found in >>>>> this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 >>>>> >>>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 >>>>> Signed-off-by: Richard W.M. Jones <rjones@redhat.com> >>>> >>>> I suppose this would also be relevant for the built-in NBD server, >>>> especially in the context of qemu-storage-daemon? >>> >>> It depends on the usage scenario really. nbdkit / qemu-nbd are >>> not commonly run under any SELinux policy, so then end up being >>> unconfined_t. A QEMU NBD client can't connect to an unconfined_t >>> socket, so we need to override it with this arg. >>> >>> In the case of qemu system emulator, under libvirt, it will >>> already have a svirt_t type, so in that case there is no need >>> to override the type for the socket. >>> >>> For qsd there's not really any strong practice established >>> but i expect most current usage is unconfined_t too and >>> would benefit from setting label. >>> >>>> If so, is this something specific to NBD sockets, or would it actually >>>> make sense to have it as a generic option in UnixSocketAddress? >>> >>> It is applicable to inet sockets too in fact. If so, should patch at least be changed to call setsockcreatecon_raw() for inet sockets as well? With current code selinux_label is silently ignored in that case. >> >> So now that 6.2 is open, should I queue the patch as is, or wait for a >> v3 that makes the option more generic to all socket usage? >> > -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-08-25 19:35 ` Eric Blake 2021-09-24 19:23 ` Eric Blake @ 2021-09-27 12:55 ` Daniel P. Berrangé 1 sibling, 0 replies; 13+ messages in thread From: Daniel P. Berrangé @ 2021-09-27 12:55 UTC (permalink / raw) To: Eric Blake Cc: Kevin Wolf, vsementsov, Richard W.M. Jones, qemu-block, qemu-devel On Wed, Aug 25, 2021 at 02:35:04PM -0500, Eric Blake wrote: > On Fri, Jul 23, 2021 at 05:38:06PM +0100, Daniel P. Berrangé wrote: > > On Fri, Jul 23, 2021 at 06:18:55PM +0200, Kevin Wolf wrote: > > > Am 23.07.2021 um 12:33 hat Richard W.M. Jones geschrieben: > > > > Under SELinux, Unix domain sockets have two labels. One is on the > > > > disk and can be set with commands such as chcon(1). There is a > > > > different label stored in memory (called the process label). This can > > > > only be set by the process creating the socket. When using SELinux + > > > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > > > you must set both labels correctly first. > > > > > > > > For qemu-nbd the options to set the second label are awkward. You can > > > > create the socket in a wrapper program and then exec into qemu-nbd. > > > > Or you could try something with LD_PRELOAD. > > > > > > > > This commit adds the ability to set the label straightforwardly on the > > > > command line, via the new --selinux-label flag. (The name of the flag > > > > is the same as the equivalent nbdkit option.) > > > > > > > > A worked example showing how to use the new option can be found in > > > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > > > > > I suppose this would also be relevant for the built-in NBD server, > > > especially in the context of qemu-storage-daemon? > > > > It depends on the usage scenario really. nbdkit / qemu-nbd are > > not commonly run under any SELinux policy, so then end up being > > unconfined_t. A QEMU NBD client can't connect to an unconfined_t > > socket, so we need to override it with this arg. > > > > In the case of qemu system emulator, under libvirt, it will > > already have a svirt_t type, so in that case there is no need > > to override the type for the socket. > > > > For qsd there's not really any strong practice established > > but i expect most current usage is unconfined_t too and > > would benefit from setting label. > > > > > If so, is this something specific to NBD sockets, or would it actually > > > make sense to have it as a generic option in UnixSocketAddress? > > > > It is applicable to inet sockets too in fact. > > So now that 6.2 is open, should I queue the patch as is, or wait for a > v3 that makes the option more generic to all socket usage? My gut feeling is that it makes sense to have a more generic option, with the selinux label specified in the "SocketAddress" QAPI type, and then have util/qemu-sockets.h be setting the context in socket_listen(). I don't think this invalidates the patch that Richard proprosed here, as we'll still need the command line argument he's added. All that will differ is that the setsockcreatecon_raw will get moved down. So from that POV, I don't think we need the general solution to be a blocker, it can be additive. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-07-23 10:33 ` Richard W.M. Jones 2021-07-23 10:47 ` Daniel P. Berrangé 2021-07-23 16:18 ` Kevin Wolf @ 2021-09-27 21:18 ` Eric Blake 2021-09-27 21:39 ` Richard W.M. Jones 2 siblings, 1 reply; 13+ messages in thread From: Eric Blake @ 2021-09-27 21:18 UTC (permalink / raw) To: Richard W.M. Jones; +Cc: vsementsov, berrange, qemu-devel, qemu-block On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote: > Under SELinux, Unix domain sockets have two labels. One is on the > disk and can be set with commands such as chcon(1). There is a > different label stored in memory (called the process label). This can > only be set by the process creating the socket. When using SELinux + > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > you must set both labels correctly first. > > For qemu-nbd the options to set the second label are awkward. You can > create the socket in a wrapper program and then exec into qemu-nbd. > Or you could try something with LD_PRELOAD. > > This commit adds the ability to set the label straightforwardly on the > command line, via the new --selinux-label flag. (The name of the flag > is the same as the equivalent nbdkit option.) > > A worked example showing how to use the new option can be found in > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > --- I'm making one tweak to your patch before sending the pull request: > +++ b/qemu-nbd.c > @@ -64,6 +68,7 @@ > #define QEMU_NBD_OPT_FORK 263 > #define QEMU_NBD_OPT_TLSAUTHZ 264 > #define QEMU_NBD_OPT_PID_FILE 265 > +#define QEMU_NBD_OPT_SELINUX_LABEL 266 > > #define MBR_SIZE 512 > > @@ -116,6 +121,9 @@ static void usage(const char *name) > " --fork fork off the server process and exit the parent\n" > " once the server is running\n" > " --pid-file=PATH store the server's process ID in the given file\n" > +#ifdef CONFIG_SELINUX > +" --selinux-label=LABEL set SELinux process label on listening socket\n" > +#endif The new option is only conditionally advertised under --help (qemu-nbd lacks a stable machine-parseable output, so scraping --help output will have to do for now)... > #if HAVE_NBD_DEVICE > "\n" > "Kernel NBD client support:\n" > @@ -532,6 +540,8 @@ int main(int argc, char **argv) > { "trace", required_argument, NULL, 'T' }, > { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, > { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE }, > + { "selinux-label", required_argument, NULL, > + QEMU_NBD_OPT_SELINUX_LABEL }, ...but is unconditionally supported as a long option even when support was not compiled in... > { NULL, 0, NULL, 0 } > }; > int ch; > @@ -558,6 +568,7 @@ int main(int argc, char **argv) > int old_stderr = -1; > unsigned socket_activation; > const char *pid_file_name = NULL; > + const char *selinux_label = NULL; > BlockExportOptions *export_opts; > > #ifdef CONFIG_POSIX > @@ -747,6 +758,9 @@ int main(int argc, char **argv) > case QEMU_NBD_OPT_PID_FILE: > pid_file_name = optarg; > break; > + case QEMU_NBD_OPT_SELINUX_LABEL: > + selinux_label = optarg; > + break; > } > } > > @@ -938,6 +952,16 @@ int main(int argc, char **argv) > } else { > backlog = MIN(shared, SOMAXCONN); > } > + if (sockpath && selinux_label) { > +#ifdef CONFIG_SELINUX > + if (setsockcreatecon_raw(selinux_label) == -1) { > + error_report("Cannot set SELinux socket create context " > + "to %s: %s", > + selinux_label, strerror(errno)); > + exit(EXIT_FAILURE); > + } > +#endif ...but here we silently ignore it if support is not compiled in. Better is to issue an error message about using an unsupported option, so I'll squash this in: diff --git i/qemu-nbd.c w/qemu-nbd.c index 5dc82c419255..94f8ec07c064 100644 --- i/qemu-nbd.c +++ w/qemu-nbd.c @@ -962,6 +962,9 @@ int main(int argc, char **argv) selinux_label, strerror(errno)); exit(EXIT_FAILURE); } +#else + error_report("SELinux support not enabled in this binary"); + exit(EXIT_FAILURE); #endif } saddr = nbd_build_socket_address(sockpath, bindto, port); @@ -978,6 +981,9 @@ int main(int argc, char **argv) strerror(errno)); exit(EXIT_FAILURE); } +#else + error_report("SELinux support not enabled in this binary"); + exit(EXIT_FAILURE); #endif } } else { -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] nbd/server: Add --selinux-label option 2021-09-27 21:18 ` Eric Blake @ 2021-09-27 21:39 ` Richard W.M. Jones 0 siblings, 0 replies; 13+ messages in thread From: Richard W.M. Jones @ 2021-09-27 21:39 UTC (permalink / raw) To: Eric Blake; +Cc: vsementsov, berrange, qemu-devel, qemu-block On Mon, Sep 27, 2021 at 04:18:34PM -0500, Eric Blake wrote: > On Fri, Jul 23, 2021 at 11:33:03AM +0100, Richard W.M. Jones wrote: > > Under SELinux, Unix domain sockets have two labels. One is on the > > disk and can be set with commands such as chcon(1). There is a > > different label stored in memory (called the process label). This can > > only be set by the process creating the socket. When using SELinux + > > SVirt and wanting qemu to be able to connect to a qemu-nbd instance, > > you must set both labels correctly first. > > > > For qemu-nbd the options to set the second label are awkward. You can > > create the socket in a wrapper program and then exec into qemu-nbd. > > Or you could try something with LD_PRELOAD. > > > > This commit adds the ability to set the label straightforwardly on the > > command line, via the new --selinux-label flag. (The name of the flag > > is the same as the equivalent nbdkit option.) > > > > A worked example showing how to use the new option can be found in > > this bug: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1984938 > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com> > > --- > > I'm making one tweak to your patch before sending the pull request: > > > +++ b/qemu-nbd.c > > @@ -64,6 +68,7 @@ > > #define QEMU_NBD_OPT_FORK 263 > > #define QEMU_NBD_OPT_TLSAUTHZ 264 > > #define QEMU_NBD_OPT_PID_FILE 265 > > +#define QEMU_NBD_OPT_SELINUX_LABEL 266 > > > > #define MBR_SIZE 512 > > > > @@ -116,6 +121,9 @@ static void usage(const char *name) > > " --fork fork off the server process and exit the parent\n" > > " once the server is running\n" > > " --pid-file=PATH store the server's process ID in the given file\n" > > +#ifdef CONFIG_SELINUX > > +" --selinux-label=LABEL set SELinux process label on listening socket\n" > > +#endif > > The new option is only conditionally advertised under --help (qemu-nbd > lacks a stable machine-parseable output, so scraping --help output > will have to do for now)... > > > #if HAVE_NBD_DEVICE > > "\n" > > "Kernel NBD client support:\n" > > @@ -532,6 +540,8 @@ int main(int argc, char **argv) > > { "trace", required_argument, NULL, 'T' }, > > { "fork", no_argument, NULL, QEMU_NBD_OPT_FORK }, > > { "pid-file", required_argument, NULL, QEMU_NBD_OPT_PID_FILE }, > > + { "selinux-label", required_argument, NULL, > > + QEMU_NBD_OPT_SELINUX_LABEL }, > > ...but is unconditionally supported as a long option even when support > was not compiled in... > > > { NULL, 0, NULL, 0 } > > }; > > int ch; > > @@ -558,6 +568,7 @@ int main(int argc, char **argv) > > int old_stderr = -1; > > unsigned socket_activation; > > const char *pid_file_name = NULL; > > + const char *selinux_label = NULL; > > BlockExportOptions *export_opts; > > > > #ifdef CONFIG_POSIX > > @@ -747,6 +758,9 @@ int main(int argc, char **argv) > > case QEMU_NBD_OPT_PID_FILE: > > pid_file_name = optarg; > > break; > > + case QEMU_NBD_OPT_SELINUX_LABEL: > > + selinux_label = optarg; > > + break; > > } > > } > > > > @@ -938,6 +952,16 @@ int main(int argc, char **argv) > > } else { > > backlog = MIN(shared, SOMAXCONN); > > } > > + if (sockpath && selinux_label) { > > +#ifdef CONFIG_SELINUX > > + if (setsockcreatecon_raw(selinux_label) == -1) { > > + error_report("Cannot set SELinux socket create context " > > + "to %s: %s", > > + selinux_label, strerror(errno)); > > + exit(EXIT_FAILURE); > > + } > > +#endif > > ...but here we silently ignore it if support is not compiled in. > Better is to issue an error message about using an unsupported option, > so I'll squash this in: > > diff --git i/qemu-nbd.c w/qemu-nbd.c > index 5dc82c419255..94f8ec07c064 100644 > --- i/qemu-nbd.c > +++ w/qemu-nbd.c > @@ -962,6 +962,9 @@ int main(int argc, char **argv) > selinux_label, strerror(errno)); > exit(EXIT_FAILURE); > } > +#else > + error_report("SELinux support not enabled in this binary"); > + exit(EXIT_FAILURE); > #endif > } > saddr = nbd_build_socket_address(sockpath, bindto, port); > @@ -978,6 +981,9 @@ int main(int argc, char **argv) > strerror(errno)); > exit(EXIT_FAILURE); > } > +#else > + error_report("SELinux support not enabled in this binary"); > + exit(EXIT_FAILURE); > #endif > } > } else { > Good idea, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-09-27 21:40 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 10:33 [PATCH v2] nbd/server: Add --selinux-label option Richard W.M. Jones 2021-07-23 10:33 ` Richard W.M. Jones 2021-07-23 10:47 ` Daniel P. Berrangé 2021-07-26 14:22 ` Eric Blake 2021-07-23 16:18 ` Kevin Wolf 2021-07-23 16:34 ` Richard W.M. Jones 2021-07-23 16:38 ` Daniel P. Berrangé 2021-08-25 19:35 ` Eric Blake 2021-09-24 19:23 ` Eric Blake 2021-09-27 12:48 ` Vladimir Sementsov-Ogievskiy 2021-09-27 12:55 ` Daniel P. Berrangé 2021-09-27 21:18 ` Eric Blake 2021-09-27 21:39 ` Richard W.M. Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).