[Libguestfs] [PATCH nbdkit v2 2/2] server: Use a thread-local pread/pwrite buffer to avoid leaking heap data.

[Richard W.M. Jones]

On Tue, Apr 23, 2019 at 10:31:06AM -0500, Eric Blake wrote:
> On 4/23/19 10:09 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.  To avoid
> > this create a thread-local data buffer which is initialized to zero
> > and expanded (with zeroes) as required.
> > 
> > This buffer is shared between pread and pwrite which makes the code a
> > little bit simpler.  Also this may improve locality by reusing the
> > same memory for all pread and pwrite calls in the same thread.
> > 
> > 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.
> 
> Could merge these last two paragraphs into one and drop the special-case
> mention of OCaml (now that the previous commit makes OCaml like all the
> other plugins).
> 
> > 
> > Of course I cannot check plugins which may be supplied by others.
> > 
> > Credit: Eric Blake for finding the bug.
> > ---
> >  server/internal.h    |  1 +
> >  server/protocol.c    | 16 +++++++++-------
> >  server/threadlocal.c | 37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 47 insertions(+), 7 deletions(-)
> > 
> 
> >      if (cmd == NBD_CMD_READ || cmd == NBD_CMD_WRITE) {
> > -      buf = malloc (count);
> > +      buf = threadlocal_buffer ((size_t) count);
> >        if (buf == NULL) {
> > -      out_of_memory:
> > -        perror ("malloc");
> >          error = ENOMEM;
> 
> Old code called perror() when nbdkit_extents_new() failed...

Yes, I believe that was a mistake.  nbdkit_extents_new already calls
nbdkit_error so calling perror isn't necessary.  Perhaps I should
split this into a separate patch.

> >          if (cmd == NBD_CMD_WRITE &&
> >              skip_over_write_buffer (conn->sockin, count) < 0)
> > @@ -673,8 +673,10 @@ protocol_recv_request_send_reply (struct connection *conn)
> >      /* Allocate the extents list for block status only. */
> >      if (cmd == NBD_CMD_BLOCK_STATUS) {
> >        extents = nbdkit_extents_new (offset, conn->exportsize);
> > -      if (extents == NULL)
> > -        goto out_of_memory;
> > +      if (extents == NULL) {
> > +        error = ENOMEM;
> > +        goto send_reply;
> > +      }
> 
> ...new code does not. nbdkit_error() might be nicer than perror()
> anyways. I'll let you decide what, if any, error reporting needs to be
> done beyond merely sending ENOMEM to the client.
> 
> 
> > +extern void *
> > +threadlocal_buffer (size_t size)
> > +{
> > +  struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
> > +
> > +  if (!threadlocal)
> > +    abort ();
> > +
> > +  if (threadlocal->buffer_size < size) {
> > +    void *ptr;
> > +
> > +    ptr = realloc (threadlocal->buffer, size);
> > +    if (ptr == NULL) {
> > +      nbdkit_error ("threadlocal_buffer: realloc: %m");
> > +      return NULL;
> > +    }
> > +    memset (ptr, 0, size);
> 
> You could memset just the tail of the enlarged buffer compared to its
> previous size, but this is fine, too.
> 
> > +    threadlocal->buffer = ptr;
> > +    threadlocal->buffer_size = size;
> > +  }
> > +
> > +  return threadlocal->buffer;
> > +}
> > 
> 
> Other than my question about extent allocation failure, LGTM.

OK, I'll split that other thing out into a separate patch, and
push it all.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW