10:53 a.m.
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