Re: [Qemu-devel] [PATCH V6 1/3] Implement sync modes for drive-backup.

[Kevin Wolf]

Am 25.07.2013 um 00:01 hat Ian Main geschrieben:
> On Wed, Jul 24, 2013 at 02:32:53PM -0600, Eric Blake wrote:
> > On 07/24/2013 04:55 AM, Kevin Wolf wrote:
> > 
> > > Unconditionally overriding format for NEW_IMAGE_MODE_EXISTING is
> > > definitely wrong. It's the user's choice which COW format to use for the
> > > backup image. There's no reason why it has to be the same format as the
> > > image that is being backed up.
> > > 
> > > Before, bs->drv->format_name was a default for the case where a new
> > > image had to be created and no format was given; and the format of
> > > existing images could be probed. This is still what makes most sense to
> > > me. What's even the goal with this change?
> 
> Actually I think that code is wrong.  If we are using
> NEW_IMAGE_MODE_EXISTING then format doesn't get used.  We just end up
> using bdrv_open() below to open the existing image.  Format should not
> be specified for an existing image.

That bdrv_open() gets drv passed, which is the block driver specified by
format. If you pass NULL instead, bdrv_open() tries to probe the format,
which is unreliable (raw is indistinguishable from other formats) and
as Eric notes has led to security problems in the past.

> > Furthermore, I'm proposing that for 1.6, we should make the format
> > argument mandatory for drive-backup.  We made it optional for
> > drive-mirror, to allow for probing, but there have been CVEs in the past
> > due to probing of a raw file gone wrong.  We can always relax a
> > mandatory argument into an optional one in 1.7, if we decide that
> > probing can be done safely, but we can never turn an optional argument
> > into a mandatory one once the initial release bakes in the option.  It
> > would make the code a lot simpler to just have a mandatory format
> > argument, instead of having to bake in and document hueristics on which
> > format is picked when the caller doesn't provide one.
> 
> So I made format mandatory in the last patch but only for
> NEW_IMAGE_MODE_ABSOLUTE_PATHS.  It actually doesn't make sense to
> specify the format of an existing image so I left it optional as an
> argument, but it will throw an error if it's not specified for the case
> where we create a new image.
> 
> That make sense?

No, see above, it's important as which format an existing image is
opened.

Kevin