Libguestfs - May 2019 - nbdkit problem with cache/cow and unaligned sizes

Right now, the cache and cow filters always round up requests to blksize
boundaries (blksize for cache is dynamically determined at connection
start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the
underlying file, but can cause problems when reading past EOF for a
partial tail of an underlying plugin. We aren't validating that filter
calls to next_ops are within bounds; and even if the plugin tolerates
the past-EOF read, we aren't guaranteeing that the client will always
read 0 bytes in the past-EOF tail.

Several ideas of fixing it, each with some drawbacks:
+ in cache/cow_get_size(), truncate the plugin's size down to blksize
prior to calling blk_set_size() (renders the plugin's tail unusable)
+ reject serving images that aren't already aligned to blksize (avoids
missing bytes or worrying about past-EOF slop, but can be mean, unless...)
+ document that for unaligned images, you can use --filter=cache
--filter=truncate round-up=BLKSIZE, to let the truncate filter take care
of our slop (doesn't play nicely with the fact that we can only use a
filter once, if a user wants to also use --filter=truncate prior to
--filter=cache)
+ rewrite both the cache/blk.c and cow/blk.c handlers to pay more
attention to unaligned EOF (code duplication)
+ teach filters.c next_ops to auto-cap filter requests into valid ranges
prior to calling into the next layer (trickier than it looks, especially
if we later add NBD resize extension support)
+ others?

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

On 5/15/19 3:11 PM, Eric Blake wrote:
> Right now, the cache and cow filters always round up requests to blksize
> boundaries (blksize for cache is dynamically determined at connection
> start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the
> underlying file, but can cause problems when reading past EOF for a
> partial tail of an underlying plugin. We aren't validating that filter
> calls to next_ops are within bounds; and even if the plugin tolerates
> the past-EOF read, we aren't guaranteeing that the client will always
> read 0 bytes in the past-EOF tail.
> 
> Several ideas of fixing it, each with some drawbacks:
> + in cache/cow_get_size(), truncate the plugin's size down to blksize
> prior to calling blk_set_size() (renders the plugin's tail unusable)
Given that the blocksize filter already truncates the plugin's size down...
> + reject serving images that aren't already aligned to blksize (avoids
> missing bytes or worrying about past-EOF slop, but can be mean, unless...)
> + document that for unaligned images, you can use --filter=cache
> --filter=truncate round-up=BLKSIZE, to let the truncate filter take care
> of our slop (doesn't play nicely with the fact that we can only use a
> filter once, if a user wants to also use --filter=truncate prior to
> --filter=cache)
...and the truncate filter already works for rounding images up instead
of down, I'm somewhat leaning towards just better documentation of the
issue and use of the truncate filter, in all three of blocksize, cache,
and cow. It still doesn't help anyone that wants to run the truncate
filter before, rather than after, the other filters, but that's less common.
> + rewrite both the cache/blk.c and cow/blk.c handlers to pay more
> attention to unaligned EOF (code duplication)
> + teach filters.c next_ops to auto-cap filter requests into valid ranges
> prior to calling into the next layer (trickier than it looks, especially
> if we later add NBD resize extension support)
> + others?
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Wed, May 15, 2019 at 03:11:42PM -0500, Eric Blake
wrote:
> Right now, the cache and cow filters always round up requests to blksize
> boundaries (blksize for cache is dynamically determined at connection
> start, for cow is fixed as BLKSIZE). Which is fine for the bulk of the
> underlying file, but can cause problems when reading past EOF for a
> partial tail of an underlying plugin. We aren't validating that filter
> calls to next_ops are within bounds; and even if the plugin tolerates
> the past-EOF read, we aren't guaranteeing that the client will always
> read 0 bytes in the past-EOF tail.
> 
> Several ideas of fixing it, each with some drawbacks:
> + in cache/cow_get_size(), truncate the plugin's size down to blksize
> prior to calling blk_set_size() (renders the plugin's tail unusable)
> + reject serving images that aren't already aligned to blksize (avoids
> missing bytes or worrying about past-EOF slop, but can be mean, unless...)
> + document that for unaligned images, you can use --filter=cache
> --filter=truncate round-up=BLKSIZE, to let the truncate filter take care
> of our slop (doesn't play nicely with the fact that we can only use a
> filter once, if a user wants to also use --filter=truncate prior to
> --filter=cache)
> + rewrite both the cache/blk.c and cow/blk.c handlers to pay more
> attention to unaligned EOF (code duplication)
> + teach filters.c next_ops to auto-cap filter requests into valid ranges
> prior to calling into the next layer (trickier than it looks, especially
> if we later add NBD resize extension support)
> + others?
So I'll just make a quick comment on this:

For plugins, the server is careful to only send requests to the plugin
which are within the bounds of the image.  Therefore plugins can
simply assume that offset/count passed in to them are correct.

However if a bad filter is placed on top of a plugin it could
incorrectly send bad bounds requests to the plugin, which could cause
mayhem.  I tried to implement checking of parameters passed between
layers, and it's a lot harder than it seems (for architectural
reasons).

Therefore we currently rely on filters being written correctly.

If seems like if the cache/cow plugins don't do this then they are
buggy.  But I've not had time to look at this in detail.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top