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

[Eric Blake]

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