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...
> 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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
