Libguestfs - Feb 2022 - [nbdkit PATCH] nbd: Opt in to libnbd pread_initialize speedup

On 02/10/22 16:38, Eric Blake wrote:
> Our nbd plugin has always properly checked for asynch errors (and thus
> has never been at risk of a vulnerability similar to CVE-2022-0485
> just fixed in nbdcopy).  What's more, the nbdkit core guarantees
> (since commit b1ce255e in 2019, v1.13.1) that the buffer handed to a
> .pread callback has been pre-sanitized to not leak heap contents,
> regardless of how buggy a plugin might be (see
> server/protocol.c:protocol_recv_request_send_reply(), which uses
> server/threadlocal.c:threadlocal_buffer() for a safe buffer).  Thus,
> we do not need libnbd to spend time re-initializing the buffer, if
> libnbd is new enough to give us that knob.
> ---
> 
> I know that Laszlo was sceptical whether any real client might want to
> use the libnbd knob [1], but I'm actually comfortable with this one.
> 
> [1]
https://listman.redhat.com/archives/libguestfs/2022-February/msg00144.html
> 
>  plugins/nbd/nbd.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
> index ae595ea7..2e154e8d 100644
> --- a/plugins/nbd/nbd.c
> +++ b/plugins/nbd/nbd.c
> @@ -1,5 +1,5 @@
>  /* nbdkit
> - * Copyright (C) 2017-2020, 2022 Red Hat Inc.
> + * Copyright (C) 2017-2022 Red Hat Inc.
>   *
>   * Redistribution and use in source and binary forms, with or without
>   * modification, are permitted provided that the following conditions are
> @@ -637,6 +637,15 @@ nbdplug_open_handle (int readonly, const char
*client_export)
>  #if LIBNBD_HAVE_NBD_SET_FULL_INFO
>    if (nbd_set_full_info (h->nbd, 1) == -1)
>      goto errnbd;
> +#endif
> +#if LIBNBD_HAVE_NBD_SET_PREAD_INITIALIZE
> +  /* nbdkit guarantees that the buffers passed to our .pread callback
> +   * are pre-initialized; and we in turn ensure that the buffer is not
> +   * dereferenced if the NBD server replied with an error.  Thus, we
> +   * are safe opting in to this libnbd speedup.
> +   */
> +  if (nbd_set_pread_initialize (h->nbd, false) == -1)
> +    goto errnbd;
>  #endif
>    if (dynamic_export && uri) {
>  #if LIBNBD_HAVE_NBD_SET_OPT_MODE
> 
hahaha, this was real quick ;)

Acked-by: Laszlo Ersek <lersek at redhat.com>

Laszlo

On Fri, Feb 11, 2022 at 07:14:30AM +0100, Laszlo Ersek
wrote:
> On 02/10/22 16:38, Eric Blake wrote:
> > Our nbd plugin has always properly checked for asynch errors (and thus
> > has never been at risk of a vulnerability similar to CVE-2022-0485
> > just fixed in nbdcopy).  What's more, the nbdkit core guarantees
> > (since commit b1ce255e in 2019, v1.13.1) that the buffer handed to a
> > .pread callback has been pre-sanitized to not leak heap contents,
> > regardless of how buggy a plugin might be (see
> > server/protocol.c:protocol_recv_request_send_reply(), which uses
> > server/threadlocal.c:threadlocal_buffer() for a safe buffer).  Thus,
> > we do not need libnbd to spend time re-initializing the buffer, if
> > libnbd is new enough to give us that knob.
> > ---
> > 
> > +#if LIBNBD_HAVE_NBD_SET_PREAD_INITIALIZE
> > +  /* nbdkit guarantees that the buffers passed to our .pread callback
> > +   * are pre-initialized; and we in turn ensure that the buffer is
not
> > +   * dereferenced if the NBD server replied with an error.  Thus, we
> > +   * are safe opting in to this libnbd speedup.
> > +   */
> > +  if (nbd_set_pread_initialize (h->nbd, false) == -1)
> > +    goto errnbd;
> >  #endif
> >    if (dynamic_export && uri) {
> >  #if LIBNBD_HAVE_NBD_SET_OPT_MODE
> > 
> 
> hahaha, this was real quick ;)
I don't know if the speedy pun was intentional ;)

Now pushed as 31997b25.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org