Re: [SPAM] [PATCH v4] 9pfs: deprecate 'proxy' backend

[Greg Kurz <groug@kaod.org>]

On Fri, 23 Jun 2023 14:41:15 +0200
Christian Schoenebeck <qemu_oss@crudebyte.com> wrote:

> As recent CVE-2023-2861 once again showed, the 9p 'proxy' fs driver is in
> bad shape. Using the 'proxy' backend was already discouraged for safety
> reasons before and we recommended to use the 'local' backend (preferably
> in conjunction with its 'mapped' security model) instead, but now it is
> time to officially deprecate the 'proxy' backend.
> 
> Signed-off-by: Christian Schoenebeck <qemu_oss@crudebyte.com>
> ---
>  v3 -> v4:
>  - MAINTAINERS: also move virtfs-proxy-helper.rst to 'obsolete' section
>  - deprecated.rst: suggest virtiofsd as alternative.
>  - deprecated.rst: mention a considerable future reimplementation of
>    'proxy' using 'vhost'.
>  - QEMU runtime warning: merge deprecation warnings of '-virtfs proxy' and
>    '-fsdev proxy' into a single deprecation warning and mention virtiofsd
>    as alternative.
>  - virtfs-proxy-helper daemon: print runtime deprecation warning here as
>    well.
>  - commit log: mention 'mapped' security model.
> 
>  MAINTAINERS                        |  9 ++++++++-
>  docs/about/deprecated.rst          | 23 +++++++++++++++++++++++
>  docs/tools/virtfs-proxy-helper.rst |  3 +++
>  fsdev/qemu-fsdev.c                 |  8 ++++++++
>  fsdev/virtfs-proxy-helper.c        |  9 +++++++++
>  hw/9pfs/9p-proxy.c                 |  5 +++++
>  hw/9pfs/9p-proxy.h                 |  5 +++++
>  meson.build                        |  2 +-
>  qemu-options.hx                    |  6 +++++-
>  9 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 436b3f0afe..3aa70b5c21 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2118,13 +2118,20 @@ S: Odd Fixes
>  W: https://wiki.qemu.org/Documentation/9p
>  F: hw/9pfs/
>  X: hw/9pfs/xen-9p*
> +X: hw/9pfs/9p-proxy*
>  F: fsdev/
> -F: docs/tools/virtfs-proxy-helper.rst
> +X: fsdev/virtfs-proxy-helper.c
>  F: tests/qtest/virtio-9p-test.c
>  F: tests/qtest/libqos/virtio-9p*
>  T: git https://gitlab.com/gkurz/qemu.git 9p-next
>  T: git https://github.com/cschoenebeck/qemu.git 9p.next
>  
> +virtio-9p-proxy
> +F: hw/9pfs/9p-proxy*
> +F: fsdev/virtfs-proxy-helper.c
> +F: docs/tools/virtfs-proxy-helper.rst
> +S: Obsolete
> +
>  virtio-blk
>  M: Stefan Hajnoczi <stefanha@redhat.com>
>  L: qemu-block@nongnu.org
> diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst
> index 0743459862..4ce75722f3 100644
> --- a/docs/about/deprecated.rst
> +++ b/docs/about/deprecated.rst
> @@ -343,6 +343,29 @@ the addition of volatile memory support, it is now necessary to distinguish
>  between persistent and volatile memory backends.  As such, memdev is deprecated
>  in favor of persistent-memdev.
>  
> +``-fsdev proxy`` and ``-virtfs proxy`` (since 8.1)
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +The 9p ``proxy`` filesystem backend driver has been deprecated and will be
> +removed (along with its proxy helper daemon) in a future version of QEMU. Please
> +use ``-fsdev local`` or ``-virtfs local`` for using the 9p ``local`` filesystem
> +backend instead, or alternatively consider deploying virtiofsd instead.

Maybe drop the first "instead" to avoid repetition ?

> +
> +The 9p ``proxy`` backend was originally developed as an alternative to the 9p
> +``local`` backend. The idea was to enhance security by dispatching actual low
> +level filesystem operations from 9p server (QEMU process) over to a separate
> +process (the virtfs-proxy-helper binary). However this alternative never gained
> +momentum. The proxy backend is much slower than the local backend, hasn't seen
> +any development in years, and showed to be less secure, especially due to the
> +fact that its helper daemon must be run as root, whereas with the local backend
> +QEMU is typically run as unprivileged user and allows to tighten behaviour by
> +mapping permissions et al by using its 'mapped' security model option.
> +
> +Nowadays it would make sense to reimplement the ``proxy`` backend by using
> +QEMU's ``vhost`` feature, which would eliminate the high latency costs under
> +which the 9p ``proxy`` backend currently suffers. However as of to date nobody
> +has indicated plans for such kind of reimplemention unfortunately.
> +
>  
>  Block device options
>  ''''''''''''''''''''
> diff --git a/docs/tools/virtfs-proxy-helper.rst b/docs/tools/virtfs-proxy-helper.rst
> index 6cdeedf8e9..bd310ebb07 100644
> --- a/docs/tools/virtfs-proxy-helper.rst
> +++ b/docs/tools/virtfs-proxy-helper.rst
> @@ -9,6 +9,9 @@ Synopsis
>  Description
>  -----------
>  
> +NOTE: The 9p 'proxy' backend is deprecated (since QEMU 8.1) and will be
> +removed, along with this daemon, in a future version of QEMU!
> +
>  Pass-through security model in QEMU 9p server needs root privilege to do
>  few file operations (like chown, chmod to any mode/uid:gid).  There are two
>  issues in pass-through security model:
> diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c
> index 3da64e9f72..9a50ee370b 100644
> --- a/fsdev/qemu-fsdev.c
> +++ b/fsdev/qemu-fsdev.c
> @@ -133,6 +133,14 @@ int qemu_fsdev_add(QemuOpts *opts, Error **errp)
>      }
>  
>      if (fsdriver) {
> +        if (strncmp(fsdriver, "proxy", 5) == 0) {
> +            warn_report(
> +                "'-fsdev proxy' and '-virtfs proxy' are deprecated, use "
> +                "'local' instead of 'proxy, or consider deploying virtiofsd "
> +                "instead"

Ditto.

LGTM.

Reviewed-by: Greg Kurz <groug@kaod.org>

> +            );
> +        }
> +
>          for (i = 0; i < ARRAY_SIZE(FsDrivers); i++) {
>              if (strcmp(FsDrivers[i].name, fsdriver) == 0) {
>                  break;
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index d9511f429c..144aaf585a 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -9,6 +9,11 @@
>   * the COPYING file in the top-level directory.
>   */
>  
> +/*
> + * NOTE: The 9p 'proxy' backend is deprecated (since QEMU 8.1) and will be
> + * removed in a future version of QEMU!
> + */
> +
>  #include "qemu/osdep.h"
>  #include <glib/gstdio.h>
>  #include <sys/resource.h>
> @@ -1057,6 +1062,10 @@ int main(int argc, char **argv)
>      struct statfs st_fs;
>  #endif
>  
> +    fprintf(stderr, "NOTE: The 9p 'proxy' backend is deprecated (since "
> +                    "QEMU 8.1) and will be removed in a future version of "
> +                    "QEMU!\n");
> +
>      prog_name = g_path_get_basename(argv[0]);
>  
>      is_daemon = true;
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 99d115ff0d..905cae6992 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -15,6 +15,11 @@
>   * https://wiki.qemu.org/Documentation/9p
>   */
>  
> +/*
> + * NOTE: The 9p 'proxy' backend is deprecated (since QEMU 8.1) and will be
> + * removed in a future version of QEMU!
> + */
> +
>  #include "qemu/osdep.h"
>  #include <sys/socket.h>
>  #include <sys/un.h>
> diff --git a/hw/9pfs/9p-proxy.h b/hw/9pfs/9p-proxy.h
> index b84301d001..9be4718d3e 100644
> --- a/hw/9pfs/9p-proxy.h
> +++ b/hw/9pfs/9p-proxy.h
> @@ -10,6 +10,11 @@
>   * the COPYING file in the top-level directory.
>   */
>  
> +/*
> + * NOTE: The 9p 'proxy' backend is deprecated (since QEMU 8.1) and will be
> + * removed in a future version of QEMU!
> + */
> +
>  #ifndef QEMU_9P_PROXY_H
>  #define QEMU_9P_PROXY_H
>  
> diff --git a/meson.build b/meson.build
> index 34306a6205..05c01b72bb 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -4170,7 +4170,7 @@ if have_block
>    summary_info += {'Block whitelist (ro)': get_option('block_drv_ro_whitelist')}
>    summary_info += {'Use block whitelist in tools': get_option('block_drv_whitelist_in_tools')}
>    summary_info += {'VirtFS (9P) support':    have_virtfs}
> -  summary_info += {'VirtFS (9P) Proxy Helper support': have_virtfs_proxy_helper}
> +  summary_info += {'VirtFS (9P) Proxy Helper support (deprecated)': have_virtfs_proxy_helper}
>    summary_info += {'Live block migration': config_host_data.get('CONFIG_LIVE_BLOCK_MIGRATION')}
>    summary_info += {'replication support': config_host_data.get('CONFIG_REPLICATION')}
>    summary_info += {'bochs support':     get_option('bochs').allowed()}
> diff --git a/qemu-options.hx b/qemu-options.hx
> index b57489d7ca..3a6c7d3ef9 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1735,7 +1735,9 @@ SRST
>          Accesses to the filesystem are done by QEMU.
>  
>      ``proxy``
> -        Accesses to the filesystem are done by virtfs-proxy-helper(1).
> +        Accesses to the filesystem are done by virtfs-proxy-helper(1). This
> +        option is deprecated (since QEMU 8.1) and will be removed in a future
> +        version of QEMU. Use ``local`` instead.
>  
>      ``synth``
>          Synthetic filesystem, only used by QTests.
> @@ -1867,6 +1869,8 @@ SRST
>  
>      ``proxy``
>          Accesses to the filesystem are done by virtfs-proxy-helper(1).
> +        This option is deprecated (since QEMU 8.1) and will be removed in a
> +        future version of QEMU. Use ``local`` instead.
>  
>      ``synth``
>          Synthetic filesystem, only used by QTests.

-- 
Greg