| Message ID | 20210930084701.3899578-1-rjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] nbd/server: Add --selinux-label option | expand |
9/30/21 11:47, 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> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> this should be Reviewed-by? > --- > configure | 8 +++- > meson.build | 10 ++++- > meson_options.txt | 3 ++ > qemu-nbd.c | 39 +++++++++++++++++++ [..] > } > > @@ -938,6 +952,19 @@ int main(int argc, char **argv) > } else { > backlog = MIN(shared, SOMAXCONN); > } > + if (sockpath && selinux_label) { 1. Why only for unix sockets? 2. If [1] is intentional, why silently ignore the new option for ip sockets, shouldn't error-out instead? > +#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); > + } > +#else > + error_report("SELinux support not enabled in this binary"); > + 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 +972,18 @@ 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); > + } > +#else > + error_report("SELinux support not enabled in this binary"); > + exit(EXIT_FAILURE); > +#endif > + } > } else { > size_t i; > /* See comment in check_socket_activation above. */ [..]
On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote: > > 9/30/21 11:47, 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> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Eric Blake <eblake@redhat.com> > > this should be Reviewed-by? Maybe, because of this: https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html I got confused with this v3.
On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: > On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy > <vsementsov@virtuozzo.com> wrote: > > > > 9/30/21 11:47, 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> > > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > > > this should be Reviewed-by? > > Maybe, because of this: > https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html > > I got confused with this v3. Yes, I'd somehow lost the original patch and picked it up from Eric's queue to make v3. Having said that I'm not sure what the objection above means. Do you mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) Rich.
9/30/21 21:37, Richard W.M. Jones wrote: > On Thu, Sep 30, 2021 at 02:00:11PM -0300, Willian Rampazzo wrote: >> On Thu, Sep 30, 2021 at 5:55 AM Vladimir Sementsov-Ogievskiy >> <vsementsov@virtuozzo.com> wrote: >>> >>> 9/30/21 11:47, 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> >>>> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> >>>> Signed-off-by: Eric Blake <eblake@redhat.com> >>> >>> this should be Reviewed-by? >> >> Maybe, because of this: >> https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg07081.html >> >> I got confused with this v3. > > Yes, I'd somehow lost the original patch and picked it up from Eric's > queue to make v3. Than it's probably correct. Still a bit strange to send own patch with another s-o-b in the end. > > Having said that I'm not sure what the objection above means. Do you > mean Eric's tag should be Reviewed-by instead of S-o-b? (And why?) > I thought you just accidentally used s-o-b instead of r-b.
On Thu, Sep 30, 2021 at 11:54:58AM +0300, Vladimir Sementsov-Ogievskiy wrote: > 9/30/21 11:47, 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> > > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> > > Signed-off-by: Eric Blake <eblake@redhat.com> > > this should be Reviewed-by? In this case, I think S-o-b is actually correct: I did make some tweaks to Rich's v2 while preparing my pull request, so Rich is crediting my addition to his work. And at the time of my pull request that included his v2 (before it got dropped for 32-bit build problems), I had not actually sent my R-b, because I was already trusting the R-b present from other reviewers. Oddly enough, even if Rich had dropped my S-o-b line, it will still eventually reappear, since I'll be queuing this patch through my NBD tree which requires me to touch it again. So already having it now doesn't hurt. [Many of the patches that go through my tree end up with both my R-b and S-o-b; but there are patches where I have S-o-b but not R-b, because I trusted the review of others, and view the act of a careful review as orthogonal from the responsibility of touching a patch enough to include it in a pull request] > > > --- > > configure | 8 +++- > > meson.build | 10 ++++- > > meson_options.txt | 3 ++ > > qemu-nbd.c | 39 +++++++++++++++++++ > > [..] > > > } > > @@ -938,6 +952,19 @@ int main(int argc, char **argv) > > } else { > > backlog = MIN(shared, SOMAXCONN); > > } > > + if (sockpath && selinux_label) { > > 1. Why only for unix sockets? > > 2. If [1] is intentional, why silently ignore the new option for ip sockets, shouldn't error-out instead? > Good point, and I missed it in v2, in spite of my touching that patch to avoid silent ignoring when selinux support was not compiled in. So at this point, I'm less certain whether it is smarter to reject --selinux-label on non-unix sockets, or whether we try to do the labeling regardless of socket type; and thus consider it premature for me to give R-b until we have that resolved.
diff --git a/configure b/configure index 1043ccce4f..b3211a66ee 100755 --- a/configure +++ b/configure @@ -445,6 +445,7 @@ fuse="auto" fuse_lseek="auto" multiprocess="auto" slirp_smbd="$default_feature" +selinux="auto" malloc_trim="auto" gio="$default_feature" @@ -1576,6 +1577,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" @@ -1963,6 +1968,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 @@ -5207,7 +5213,7 @@ 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\ + -Dselinux=$selinux \ $(if test "$default_feature" = no; then echo "-Dauto_features=disabled"; fi) \ -Dtcg_interpreter=$tcg_interpreter \ $cross_arg \ diff --git a/meson.build b/meson.build index 15ef4d3c41..0ded2ac5eb 100644 --- a/meson.build +++ b/meson.build @@ -1072,6 +1072,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 = [] @@ -1300,6 +1305,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]) @@ -2759,7 +2765,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') @@ -3124,6 +3131,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 65ebec598f..8da6cd90bb 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,19 @@ 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); + } +#else + error_report("SELinux support not enabled in this binary"); + 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 +972,18 @@ 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); + } +#else + error_report("SELinux support not enabled in this binary"); + 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-i386-cross.docker b/tests/docker/dockerfiles/fedora-i386-cross.docker index dbb8195eb1..91a7c51614 100644 --- a/tests/docker/dockerfiles/fedora-i386-cross.docker +++ b/tests/docker/dockerfiles/fedora-i386-cross.docker @@ -7,6 +7,7 @@ ENV PACKAGES \ gcc \ git \ libffi-devel.i686 \ + libselinux-devel.i686 \ libtasn1-devel.i686 \ libzstd-devel.i686 \ make \ 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 \