Libguestfs - Apr 2019 - Re: [PATCH nbdkit 2/2] server: Zero the read buffer before passing it to plugin .pread method.

On 4/23/19 4:01 AM, Richard W.M. Jones wrote:
> If the plugin .pread method did not fill the buffer with data then the
> contents of the heap could be leaked back to the client.  The simplest
> way to avoid any possibility of this happening is to zero the buffer
> before passing it to the .pread method.
> 
> I have checked plugins shipped with nbdkit and none of them are
> vulnerable in this way.  They all do one of these things:
> 
>  - They fully set the buffer with a single call to something like
>    memcpy, memset, etc.
> 
>  - They use a loop like ‘while (count > 0)’ and correctly update count
>    and the buffer pointer on each iteration.
> 
>  - For language plugins (except OCaml), they check that the string
>    length returned from the language plugin is at least as long as the
>    requested data, before memcpy-ing the data to the return buffer.
> 
>  - For OCaml, see the previous commit.
> 
> Of course I cannot check plugins which may be supplied by others.
> 
> Credit: Eric Blake for finding the bug.
> ---
>  server/protocol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
This patch does not document any guarantee. Would it be worth explicitly
mentioning this behavior as part of the API guarantee of .pread, so that
plugins like zero and null can rely on the incoming buffer being
pre-zeroed rather than having to repeat the memset() themselves?

Should this behavior have an opt-out switch for plugins concerned with
speed? For safety, we want this behavior to be on by default (as we
don't know whether out-of-tree plugins might otherwise leak our heap
data). But if we were to add a new field to include/nbd-plugin.h that a
plugin can set when they DON'T want to waste time on the pre-emptive
clear because they promise to fill the entire buffer, then the bug
shifts out of nbdkit for leaking data and over to the plugin for setting
the flag incorrectly if it breaks the promise. The new field won't be
set by any existing out-of-tree plugins, and newly written plugins will
have the documentation of that particular aspect to make their decision
on whether to opt-out.
> diff --git a/server/protocol.c b/server/protocol.c
> index 54d8adb..4d67392 100644
> --- a/server/protocol.c
> +++ b/server/protocol.c
> @@ -658,10 +658,10 @@ protocol_recv_request_send_reply (struct connection
*conn)
>  
>      /* Allocate the data buffer used for either read or write requests. */
>      if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) {
> -      buf = malloc (count);
> +      buf = calloc (1, count);
Definitely a safety fix for READ, but isn't this a speed penalty for
WRITE?  That is, there is a cost of zeroing the buffer (especially if it
is a 32M request, where zeroing it may be large enough to also have
performance impacts due to cache pressure), when we KNOW that we are
going to immediately rewrite the buffer with incoming data before ever
calling into the plugin's .pwrite, makes it seem like we should limit
the calloc or memset to just the READ path.  I don't know if the added
time spent on calloc() is significant, or if it is in the noise compared
to other overheads such as the socket traffic, but it may be worth
benchmarking (I know you have your fio patches that might help us answer
that question).
>        if (buf == NULL) {
>        out_of_memory:
> -        perror ("malloc");
> +        perror ("calloc");
>          error = ENOMEM;
>          if (cmd == NBD_CMD_WRITE &&
>              skip_over_write_buffer (conn->sockin, count) < 0)
> 
Is there any smarter algorithm we could use? Right now, we are doing a
malloc()/free() per READ/WRITE. And although libc malloc() should in
general ty to be efficient on frequent malloc reuse, we can often still
be faster by reusing allocations rather than doing frequent
malloc()/free().  Would it be worth exploring a patch that creates a
per-thread buffer (maybe initially sized at 64k, and widened only as the
client sends larger requests), where we reuse the buffer between
successive calls rather than reallocating?  We'd still want to initially
zero the buffer when we first allocate one for the thread or when we
enlarge it, but we can argue that after that point, a plugin with a
.pread that does a partial job will only leak data that has already been
previously read from or written to that plugin (and no longer risk a
leak of OUR heap data), which means we avoid both the malloc() and the
memset() in the common case, for slightly faster behavior, and with no
visible difference in output for a plugin that fully fills the buffer in
.pread.

[I still hope to someday improve the nbdkit threading to create helper
threads on demand due to actual client parallel requests, rather than
its current behavior of spawning all parallel threads up front when the
client first connects - that becomes more important if we are also
allocating up to 64M in per-thread buffers, when we don't need the
allocation for a client that only ever sends serialized requests]

And if we DO switch to per-thread reused buffers, it may STILL make
sense to have nbdkit-plugin.h have a way for plugins to opt-in or
opt-out of whether .pread is guaranteed a pre-zeroed buffer on all reads.

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

I agree we should only be zeroing this buffer on NBD_CMD_READ, so the
patch is wrong as it stands.

Having an "I promise not to be bad!" flag I think just adds more
complexity to plugins.  It would be nice to do the best thing
automatically.

If we have a per-thread buffer then we're still (potentially) leaking
data between clients, even if that data only consists of previously
read data from another part of the disk.  However this does seem like
the least bad approach since (a) we're not leaking random heap data
like secret keys and (b) we don't need to make the plugin API any more
complicated.  I'll see how easy this is to implement ...

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On 4/23/19 8:34 AM, Richard W.M. Jones wrote:
> I agree we should only be zeroing this buffer on NBD_CMD_READ, so the
> patch is wrong as it stands.
> 
> Having an "I promise not to be bad!" flag I think just adds more
> complexity to plugins.  It would be nice to do the best thing
> automatically.
> 
> If we have a per-thread buffer then we're still (potentially) leaking
> data between clients, even if that data only consists of previously
> read data from another part of the disk.  However this does seem like
> the least bad approach since (a) we're not leaking random heap data
> like secret keys and (b) we don't need to make the plugin API any more
> complicated.  I'll see how easy this is to implement ...
Right now, we're firing up separate threads for separate clients - a
given thread is only reused by a single client.  (Observe a trace of the
file module with two simultaneous clients - our default of spawning 16
threads per connection means 32 helper threads are active at once when
there are two clients).

Of course, if we DO switch to a smarter thread-pooling approach (where
we put a ceiling of at most 16 outstanding requests from a given client,
but only add threads as we actually get enough parallel requests), then
we may indeed start having to worry about reusing a thread to service
requests from more than one client.

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