Re: [PATCH v2] nbd/server: Add --selinux-label option

[Richard W.M. Jones]

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/