Re: [Qemu-block] [PATCH v2 12/17] mirror: Fix potential use-after-free in active commit

[Kevin Wolf]

Am 19.09.2018 um 01:48 hat Max Reitz geschrieben:
> On 18.09.18 17:04, Kevin Wolf wrote:
> > Am 18.09.2018 um 16:11 hat Max Reitz geschrieben:
> >> The user cannot do any graph manipulation then because the monitor is
> >> blocked by starting the job.  So the only issue would be other
> >> operations that can make nodes go away without user intervention.
> >> Naturally the way to prevent conflicts would be with blockers, and
> >> currently our old job blockers should be sufficient to prevent them.
> >> But in theory we don't like them, so is this what we need the graph
> >> manipulation blocker for? :-)
> > 
> > I don't think so. Permissions are for users that are potentially
> > attached for a long time, not for protecting operations during which we
> > hold the BQL anyway.
> > 
> > Our problem here isn't that the graph is changing, but that nodes are
> > going away.
> 
> Hm, yeah.  Which they usually wouldn't unless the graph is changing,
> but that's not a necessity.

My point was more that you can modify the graph without nodes going
away (if they are still referenced somewhere else).

> Though the better explanation is: The node can only go away when it has
> only one parent.  So even if that parent took the graph manipulation
> blocker, there'd be nobody to care.
> 
> > The solution is to take references when we need them instead
> > of relying on someone else holding a reference for us when we don't know
> > who that someone is.
> 
> Yeah.  Which means that this patch isn't completely wrong, we should
> just do it for every job.  And maybe take the references not in
> block/mirror.c, but immediately in blockdev.c.

Yes. I'll drop this patch anyway as it's unrelated and making the fix in
the wrong place. As you say, blockdev.c would be the right place for a
fix, and it would be good to have a test case that actually reproduces
the problem.

> > Specifically, QMP commands might need to hold an
> > additional reference while working with a node.
> > 
> > An actual blocker for graph modification might make sense in the case of
> > a commit job, for example: If you break the backing chain while a commit
> > is running, the job becomes semantically invalid.
> 
> Well, at least doing the graph changes after the job is done will be
> impossible, yes.

Actually, it depends. Modifying the graph is okay as long as certain
conditions are met. In the case of commit that is primarily that the
base node is still in the backing chain. If you want to add a throttling
filter in the backing chain while commit is running, that's not really
a problem (other than the filter going away when commit completes...)

The "I promise this is the same" case is another option, though rather
unlikely when the base image isn't read-only, but written to by the
commit job. Maybe if you open a raw image through two different links to
the same image or something...

> (The copy operation itself should still be OK as long as the visible
> content of the top node doesn't change -- and at least in theory we only
> allow changing the backing file to something that won't violate this
> condition.  Otherwise, you'd probably have to un-share consistent_read.)

If the old and the new backing file node aren't actually referring to the
same storage, the copy operation would seemingly succeed, but not make
any sense because you wouldn't know which part of the image was committed
to which backing file.

Kevin

signature.asc
Description: PGP signature