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

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.

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;

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; + }

+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);

+ threadlocal->buffer = ptr; + threadlocal->buffer_size = size; + } + + return threadlocal->buffer; +}