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