| Message ID | 1394643643-27122-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/12/2014 11:00 AM, Markus Armbruster wrote: > Opening an encrypted image takes an additional step: setting the key. > Between open and the key set, the image must not be used. > > We have some protection against accidental use in place: you can't > unpause a guest while we're missing keys. You can, however, hot-plug > block devices lacking keys into a running guest just fine, or insert > media lacking keys. In the latter case, notifying the guest of the > insert is delayed until the key is set, which may suffice to protect > at least some guests in common usage. > > This patch makes the protection apply in more cases, in a rather > heavy-handed way: it doesn't let you open encrypted images unless > we're in a paused state. > > It doesn't extend the protection to users other than the guest (block > jobs?). Use of runstate_check() from block.c is disgusting. Best I > can do right now. Better than what we had before, and worth having in 2.0 > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 8 +++++++- > stubs/Makefile.objs | 1 + > stubs/runstate-check.c | 6 ++++++ > 3 files changed, 14 insertions(+), 1 deletion(-) > create mode 100644 stubs/runstate-check.c > Reviewed-by: Eric Blake <eblake@redhat.com>
On Wed, 03/12 18:00, Markus Armbruster wrote: > Opening an encrypted image takes an additional step: setting the key. > Between open and the key set, the image must not be used. > > We have some protection against accidental use in place: you can't > unpause a guest while we're missing keys. You can, however, hot-plug > block devices lacking keys into a running guest just fine, or insert > media lacking keys. In the latter case, notifying the guest of the > insert is delayed until the key is set, which may suffice to protect > at least some guests in common usage. > > This patch makes the protection apply in more cases, in a rather > heavy-handed way: it doesn't let you open encrypted images unless > we're in a paused state. > > It doesn't extend the protection to users other than the guest (block > jobs?). Use of runstate_check() from block.c is disgusting. Best I > can do right now. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block.c | 8 +++++++- > stubs/Makefile.objs | 1 + > stubs/runstate-check.c | 6 ++++++ > 3 files changed, 14 insertions(+), 1 deletion(-) > create mode 100644 stubs/runstate-check.c > > diff --git a/block.c b/block.c > index f1ef4b0..7604881 100644 > --- a/block.c > +++ b/block.c > @@ -1388,12 +1388,18 @@ done: > ret = -EINVAL; > goto close_and_fail; > } > - QDECREF(options); > > if (!bdrv_key_required(bs)) { > bdrv_dev_change_media_cb(bs, true); > + } else if (!runstate_check(RUN_STATE_PRELAUNCH) > + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ > + error_setg(errp, > + "Guest must be stopped for opening of encrypted image"); Changing error message here breaks qemu-iotests 087. Thanks, Fam > + ret = -EBUSY; > + goto close_and_fail; > } > > + QDECREF(options); > *pbs = bs; > return 0; > > diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs > index df3aa7a..09e7790 100644 > --- a/stubs/Makefile.objs > +++ b/stubs/Makefile.objs > @@ -19,6 +19,7 @@ stub-obj-y += mon-protocol-event.o > stub-obj-y += mon-set-error.o > stub-obj-y += pci-drive-hot-add.o > stub-obj-y += reset.o > +stub-obj-y += runstate-check.o > stub-obj-y += set-fd-handler.o > stub-obj-y += slirp.o > stub-obj-y += sysbus.o > diff --git a/stubs/runstate-check.c b/stubs/runstate-check.c > new file mode 100644 > index 0000000..bd2e375 > --- /dev/null > +++ b/stubs/runstate-check.c > @@ -0,0 +1,6 @@ > +#include "sysemu/sysemu.h" > + > +bool runstate_check(RunState state) > +{ > + return state == RUN_STATE_PRELAUNCH; > +} > -- > 1.8.1.4 > >
Il 12/03/2014 18:00, Markus Armbruster ha scritto: > + } else if (!runstate_check(RUN_STATE_PRELAUNCH) > + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ Why not "if (runstate_is_running())"? Paolo > + error_setg(errp, > + "Guest must be stopped for opening of encrypted image"); > + ret = -EBUSY; > + goto close_and_fail;
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 12/03/2014 18:00, Markus Armbruster ha scritto: >> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ > > Why not "if (runstate_is_running())"? The predicate actually wanted here is "monitor command 'cont' required to get the guest running", because 'cont' is where the protection is. My run state test is a crude approximation.
Fam Zheng <famz@redhat.com> writes: > On Wed, 03/12 18:00, Markus Armbruster wrote: >> Opening an encrypted image takes an additional step: setting the key. >> Between open and the key set, the image must not be used. >> >> We have some protection against accidental use in place: you can't >> unpause a guest while we're missing keys. You can, however, hot-plug >> block devices lacking keys into a running guest just fine, or insert >> media lacking keys. In the latter case, notifying the guest of the >> insert is delayed until the key is set, which may suffice to protect >> at least some guests in common usage. >> >> This patch makes the protection apply in more cases, in a rather >> heavy-handed way: it doesn't let you open encrypted images unless >> we're in a paused state. >> >> It doesn't extend the protection to users other than the guest (block >> jobs?). Use of runstate_check() from block.c is disgusting. Best I >> can do right now. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block.c | 8 +++++++- >> stubs/Makefile.objs | 1 + >> stubs/runstate-check.c | 6 ++++++ >> 3 files changed, 14 insertions(+), 1 deletion(-) >> create mode 100644 stubs/runstate-check.c >> >> diff --git a/block.c b/block.c >> index f1ef4b0..7604881 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1388,12 +1388,18 @@ done: >> ret = -EINVAL; >> goto close_and_fail; >> } >> - QDECREF(options); >> >> if (!bdrv_key_required(bs)) { >> bdrv_dev_change_media_cb(bs, true); >> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >> + error_setg(errp, >> + "Guest must be stopped for opening of encrypted image"); > > Changing error message here breaks qemu-iotests 087. Crap. I'm on vacation until Monday, just checking in to shepherd this patch... On *master*, "make check-block" reports Not run: 016 052 059 064 070 077 Failures: 085 087 Failed 2 of 34 tests What am I doing wrong?
On 03/13/2014 04:43 AM, Paolo Bonzini wrote: > Il 12/03/2014 18:00, Markus Armbruster ha scritto: >> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ > > Why not "if (runstate_is_running())"? Because that lacks PRELAUNCH, but PRELAUNCH also needs the protection.
Il 13/03/2014 14:18, Markus Armbruster ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 12/03/2014 18:00, Markus Armbruster ha scritto: >>> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >>> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >> >> Why not "if (runstate_is_running())"? > > The predicate actually wanted here is "monitor command 'cont' required > to get the guest running", because 'cont' is where the protection is. > My run state test is a crude approximation. > Got it. Then you need to add at least a check for "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming migration. Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are problematic, but I understand why you preferred a conservative test (sufficient condition, not necessary). You are singling out prelaunch and inmigrate because drive_init will reset autostart to 0 for an encrypted image, right? Paolo
Il 13/03/2014 14:27, Eric Blake ha scritto: >>> >> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >>> >> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >> > >> > Why not "if (runstate_is_running())"? > Because that lacks PRELAUNCH, but PRELAUNCH also needs the protection. Nope, PRELAUNCH does *not* need the protection. if (!runstate_check(PRELAUNCH) && !runstate_check(PAUSED)) error gives an error if runstate == anything *but* PRELAUNCH and PAUSED It's at least DEBUG, SAVE_VM and RESTORE_VM that do need it but are not included in runstate_is_running. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 13/03/2014 14:18, Markus Armbruster ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 12/03/2014 18:00, Markus Armbruster ha scritto: >>>> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >>>> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >>> >>> Why not "if (runstate_is_running())"? >> >> The predicate actually wanted here is "monitor command 'cont' required >> to get the guest running", because 'cont' is where the protection is. >> My run state test is a crude approximation. >> > > Got it. Then you need to add at least a check for > "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming > migration. You're right: main() goes from RUN_STATE_PRELAUNCH to RUN_STATE_INMIGRATE right when it sees -incoming. > Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are > problematic, but I understand why you preferred a conservative > test (sufficient condition, not necessary). Exactly. > You are singling out prelaunch and inmigrate because drive_init > will reset autostart to 0 for an encrypted image, right? Yes.
Il 13/03/2014 16:00, Markus Armbruster ha scritto: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> Il 13/03/2014 14:18, Markus Armbruster ha scritto: >>> Paolo Bonzini <pbonzini@redhat.com> writes: >>> >>>> Il 12/03/2014 18:00, Markus Armbruster ha scritto: >>>>> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >>>>> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >>>> >>>> Why not "if (runstate_is_running())"? >>> >>> The predicate actually wanted here is "monitor command 'cont' required >>> to get the guest running", because 'cont' is where the protection is. >>> My run state test is a crude approximation. >>> >> >> Got it. Then you need to add at least a check for >> "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming >> migration. > > You're right: main() goes from RUN_STATE_PRELAUNCH to > RUN_STATE_INMIGRATE right when it sees -incoming. > >> Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are >> problematic, but I understand why you preferred a conservative >> test (sufficient condition, not necessary). > > Exactly. > >> You are singling out prelaunch and inmigrate because drive_init >> will reset autostart to 0 for an encrypted image, right? > > Yes. Then with the check modified, Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 13/03/2014 16:00, Markus Armbruster ha scritto: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 13/03/2014 14:18, Markus Armbruster ha scritto: >>>> Paolo Bonzini <pbonzini@redhat.com> writes: >>>> >>>>> Il 12/03/2014 18:00, Markus Armbruster ha scritto: >>>>>> + } else if (!runstate_check(RUN_STATE_PRELAUNCH) >>>>>> + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ >>>>> >>>>> Why not "if (runstate_is_running())"? >>>> >>>> The predicate actually wanted here is "monitor command 'cont' required >>>> to get the guest running", because 'cont' is where the protection is. >>>> My run state test is a crude approximation. >>>> >>> >>> Got it. Then you need to add at least a check for >>> "runstate_check(RUN_STATE_INMIGRATE)", otherwise you break incoming >>> migration. >> >> You're right: main() goes from RUN_STATE_PRELAUNCH to >> RUN_STATE_INMIGRATE right when it sees -incoming. >> >>> Actually, I think only SAVE_VM/RESTORE_VM/DEBUG are >>> problematic, but I understand why you preferred a conservative >>> test (sufficient condition, not necessary). >> >> Exactly. >> >>> You are singling out prelaunch and inmigrate because drive_init >>> will reset autostart to 0 for an encrypted image, right? >> >> Yes. > > Then with the check modified, > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Will do. I'm refraining from adding your R-by, because I need to update tests, too. Thanks!
diff --git a/block.c b/block.c index f1ef4b0..7604881 100644 --- a/block.c +++ b/block.c @@ -1388,12 +1388,18 @@ done: ret = -EINVAL; goto close_and_fail; } - QDECREF(options); if (!bdrv_key_required(bs)) { bdrv_dev_change_media_cb(bs, true); + } else if (!runstate_check(RUN_STATE_PRELAUNCH) + && !runstate_check(RUN_STATE_PAUSED)) { /* HACK */ + error_setg(errp, + "Guest must be stopped for opening of encrypted image"); + ret = -EBUSY; + goto close_and_fail; } + QDECREF(options); *pbs = bs; return 0; diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index df3aa7a..09e7790 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -19,6 +19,7 @@ stub-obj-y += mon-protocol-event.o stub-obj-y += mon-set-error.o stub-obj-y += pci-drive-hot-add.o stub-obj-y += reset.o +stub-obj-y += runstate-check.o stub-obj-y += set-fd-handler.o stub-obj-y += slirp.o stub-obj-y += sysbus.o diff --git a/stubs/runstate-check.c b/stubs/runstate-check.c new file mode 100644 index 0000000..bd2e375 --- /dev/null +++ b/stubs/runstate-check.c @@ -0,0 +1,6 @@ +#include "sysemu/sysemu.h" + +bool runstate_check(RunState state) +{ + return state == RUN_STATE_PRELAUNCH; +}
Opening an encrypted image takes an additional step: setting the key. Between open and the key set, the image must not be used. We have some protection against accidental use in place: you can't unpause a guest while we're missing keys. You can, however, hot-plug block devices lacking keys into a running guest just fine, or insert media lacking keys. In the latter case, notifying the guest of the insert is delayed until the key is set, which may suffice to protect at least some guests in common usage. This patch makes the protection apply in more cases, in a rather heavy-handed way: it doesn't let you open encrypted images unless we're in a paused state. It doesn't extend the protection to users other than the guest (block jobs?). Use of runstate_check() from block.c is disgusting. Best I can do right now. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block.c | 8 +++++++- stubs/Makefile.objs | 1 + stubs/runstate-check.c | 6 ++++++ 3 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 stubs/runstate-check.c