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