[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 4/7] ios: Drop unused seek/tell callbacks.

From: Eric Blake
Subject: Re: [PATCH 4/7] ios: Drop unused seek/tell callbacks.
Date: Sat, 29 Feb 2020 05:42:46 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

On 2/29/20 5:12 AM, Eric Blake wrote:
Now that the previous patch proved we were always passing a valid
offset, it's time to shift the burden on who does seeking.  Instead of
the ios layer making two calls into the driver (first to seek, then to
do I/O), the seek is now local to the driver.  For memory (and the
upcoming NBD driver), this is actually simpler (no need to track an
artificial current location).  For file, we temporarily have more
calls to fseeko than previously (8 times instead of 1 when reading an
aligned uint64_t, for example); but that will be cleaned up in
upcoming patches when we switch to a pread-style interface.  Besides,
it would be a lame libc that didn't optimize an fseeko() that ends up
in the same location that it is already at, so we aren't really
resulting in more lseek() syscalls.

Just noticed this:

+++ b/src/ios-dev-mem.c

@@ -96,42 +93,15 @@ ios_dev_mem_putc (void *iod, int c, ios_dev_off offset)
    struct ios_dev_mem *mio = iod;

-  assert (mio->cur == offset);
-  if (mio->cur >= mio->size)
+  if (offset >= mio->size) {
+    assert (MEM_STEP > offset - mio->size);
      mio->pointer = xrealloc (mio->pointer,
                               mio->size + MEM_STEP);
-  mio->pointer[mio->cur++] = c;
+  }
+  mio->pointer[offset] = c;
    return c;

The old code assumes that mio->cur lies within mio->size + MEM_STEP, but if that was not true, it silently allows a malicious binary to request an offset to anywhere in memory outside the bounds of the IOS. My new code tries to protect itself with an assert(), but that is a denial of service (poke shouldn't exit from an assertion failure when it can instead provide a useful error message). The old code was unbounded because:


-static ios_dev_off
-ios_dev_mem_tell (void *iod)
-  struct ios_dev_mem *mio = iod;
-  return mio->cur;
-static int
-ios_dev_mem_seek (void *iod, ios_dev_off offset, int whence)
-  struct ios_dev_mem *mio = iod;
-  switch (whence)
-    {
-    case IOD_SEEK_SET:
-      mio->cur = offset;
-      break;
-    case IOD_SEEK_CUR:
-      mio->cur += offset;
-      break;
-    case IOD_SEEK_END:
-      mio->cur = mio->size - offset;
-      break;
-    }
-  return mio->cur;

we never validated that mio->cur is still reasonable. We may need to further clean up how this code behaves with unexpected offsets - I know the intent is to allow the memory buffer to auto-grow for writing out a reasonable binary, but that automatic growth has to be careful of untrusted offsets. The file driver may have similar problems. My nbd driver patches are immune because the NBD protocol doesn't support resize (yet).

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

reply via email to

[Prev in Thread] Current Thread [Next in Thread]