| Message ID | 20180507033214.19219-2-zyimin@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Bug: Sandbox: libvirt breakdowns qemu guest | expand |
On 07/05/2018 - 11:32:14, Yi Min Zhao wrote: > If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains > complied. This would make libvirt set the corresponding capability and > then trigger the guest startup fails. So let's wrap the options with > CONFIG_SECCOMP. > > Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> > --- > vl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vl.c b/vl.c > index fce1fd12d8..cb07b19c02 100644 > --- a/vl.c > +++ b/vl.c > @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "enable", > .type = QEMU_OPT_BOOL, > }, > +#ifdef CONFIG_SECCOMP > { > .name = "obsolete", > .type = QEMU_OPT_STRING, > @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "resourcecontrol", > .type = QEMU_OPT_STRING, > }, > +#endif > { /* end of list */ } > }, > }; > -- > 2.15.1 (Apple Git-101) > Acked-by: Eduardo Otubo <otubo@redhat.com>
在 2018/5/7 下午6:31, Eduardo Otubo 写道: > On 07/05/2018 - 11:32:14, Yi Min Zhao wrote: >> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains >> complied. This would make libvirt set the corresponding capability and >> then trigger the guest startup fails. So let's wrap the options with >> CONFIG_SECCOMP. >> >> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> >> --- >> vl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index fce1fd12d8..cb07b19c02 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "enable", >> .type = QEMU_OPT_BOOL, >> }, >> +#ifdef CONFIG_SECCOMP >> { >> .name = "obsolete", >> .type = QEMU_OPT_STRING, >> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "resourcecontrol", >> .type = QEMU_OPT_STRING, >> }, >> +#endif >> { /* end of list */ } >> }, >> }; >> -- >> 2.15.1 (Apple Git-101) >> > Acked-by: Eduardo Otubo <otubo@redhat.com> > > Thanks for your review!
On 05/06/2018 10:32 PM, Yi Min Zhao wrote: In the subject line: s/avoid to compile/avoid compiling/ > If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains s/elevator/elevate/ > complied. This would make libvirt set the corresponding capability and s/complied/compiled/ > then trigger the guest startup fails. So let's wrap the options with > CONFIG_SECCOMP. > > Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> > --- > vl.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vl.c b/vl.c > index fce1fd12d8..cb07b19c02 100644 > --- a/vl.c > +++ b/vl.c > @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "enable", > .type = QEMU_OPT_BOOL, > }, > +#ifdef CONFIG_SECCOMP > { > .name = "obsolete", > .type = QEMU_OPT_STRING, > @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "resourcecontrol", > .type = QEMU_OPT_STRING, > }, > +#endif The commit message mentions only 'elevateprivileges' (once the typo is fixed), but you are also crippling 'obsolete', 'spawn', and 'resourcecontrol'. Perhaps the commit message should call that out better? Or, since libvirt is looking at just 'elevateprivileges', per this line in libvirt's qemu_capabilities.c: src/qemu/qemu_capabilities.c: { "sandbox", "elevateprivileges", QEMU_CAPS_SECCOMP_BLACKLIST }, is it sufficient to just mask out that one option?
在 2018/5/8 上午2:04, Eric Blake 写道: > On 05/06/2018 10:32 PM, Yi Min Zhao wrote: > > In the subject line: s/avoid to compile/avoid compiling/ > >> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains > > s/elevator/elevate/ > >> complied. This would make libvirt set the corresponding capability and > > s/complied/compiled/ Thanks for your correction. > >> then trigger the guest startup fails. So let's wrap the options with >> CONFIG_SECCOMP. >> >> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> >> --- >> vl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index fce1fd12d8..cb07b19c02 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "enable", >> .type = QEMU_OPT_BOOL, >> }, >> +#ifdef CONFIG_SECCOMP >> { >> .name = "obsolete", >> .type = QEMU_OPT_STRING, >> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "resourcecontrol", >> .type = QEMU_OPT_STRING, >> }, >> +#endif > > The commit message mentions only 'elevateprivileges' (once the typo is > fixed), but you are also crippling 'obsolete', 'spawn', and > 'resourcecontrol'. Perhaps the commit message should call that out > better? Or, since libvirt is looking at just 'elevateprivileges', per > this line in libvirt's qemu_capabilities.c: > > src/qemu/qemu_capabilities.c: { "sandbox", "elevateprivileges", > QEMU_CAPS_SECCOMP_BLACKLIST }, > > is it sufficient to just mask out that one option? > The problem is caused by 'elevateprivileges' ostensibly from Libvirt point of view. But in Qemu, CONFIG_SECCOMP controls other options except 'enable' in parse_sandbox(). So if SECCOMP is not configured, we don't need other options instead of only elevateprivileges.
On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote: > On 05/06/2018 10:32 PM, Yi Min Zhao wrote: > > In the subject line: s/avoid to compile/avoid compiling/ > > > If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains > > s/elevator/elevate/ > > > complied. This would make libvirt set the corresponding capability and > > s/complied/compiled/ > > > then trigger the guest startup fails. So let's wrap the options with > > CONFIG_SECCOMP. > > > > Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> > > --- > > vl.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/vl.c b/vl.c > > index fce1fd12d8..cb07b19c02 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { > > .name = "enable", > > .type = QEMU_OPT_BOOL, > > }, > > +#ifdef CONFIG_SECCOMP > > { > > .name = "obsolete", > > .type = QEMU_OPT_STRING, > > @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { > > .name = "resourcecontrol", > > .type = QEMU_OPT_STRING, > > }, > > +#endif > > The commit message mentions only 'elevateprivileges' (once the typo is > fixed), but you are also crippling 'obsolete', 'spawn', and > 'resourcecontrol'. Perhaps the commit message should call that out better? > Or, since libvirt is looking at just 'elevateprivileges', per this line in > libvirt's qemu_capabilities.c: > > src/qemu/qemu_capabilities.c: { "sandbox", "elevateprivileges", > QEMU_CAPS_SECCOMP_BLACKLIST }, > > is it sufficient to just mask out that one option? If seccomp is disabled, we should really disable the entire -sandbox argument, not merly the options to it. Regards, Daniel
在 2018/5/8 下午6:37, Daniel P. Berrangé 写道: > On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote: >> On 05/06/2018 10:32 PM, Yi Min Zhao wrote: >> >> In the subject line: s/avoid to compile/avoid compiling/ >> >>> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains >> s/elevator/elevate/ >> >>> complied. This would make libvirt set the corresponding capability and >> s/complied/compiled/ >> >>> then trigger the guest startup fails. So let's wrap the options with >>> CONFIG_SECCOMP. >>> >>> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> >>> --- >>> vl.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/vl.c b/vl.c >>> index fce1fd12d8..cb07b19c02 100644 >>> --- a/vl.c >>> +++ b/vl.c >>> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { >>> .name = "enable", >>> .type = QEMU_OPT_BOOL, >>> }, >>> +#ifdef CONFIG_SECCOMP >>> { >>> .name = "obsolete", >>> .type = QEMU_OPT_STRING, >>> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { >>> .name = "resourcecontrol", >>> .type = QEMU_OPT_STRING, >>> }, >>> +#endif >> The commit message mentions only 'elevateprivileges' (once the typo is >> fixed), but you are also crippling 'obsolete', 'spawn', and >> 'resourcecontrol'. Perhaps the commit message should call that out better? >> Or, since libvirt is looking at just 'elevateprivileges', per this line in >> libvirt's qemu_capabilities.c: >> >> src/qemu/qemu_capabilities.c: { "sandbox", "elevateprivileges", >> QEMU_CAPS_SECCOMP_BLACKLIST }, >> >> is it sufficient to just mask out that one option? > If seccomp is disabled, we should really disable the entire -sandbox > argument, not merly the options to it. I think it would bring a lot of changes if disable the entire -sandbox argument. Looking from current code, sandbox is a default qemu option group, and sandbox.enable is false by default unless you obviously define it with true. So, this patch is an easier way to fixup. > > Regards, > Daniel
On 05/08/2018 11:40 PM, Yi Min Zhao wrote: >> If seccomp is disabled, we should really disable the entire -sandbox >> argument, not merly the options to it. > I think it would bring a lot of changes if disable the entire -sandbox > argument. > Looking from current code, sandbox is a default qemu option group, and > sandbox.enable is false by default unless you obviously define it with > true. > So, this patch is an easier way to fixup. If the only thing you can do with -sandbox is turn it off (which is its default state), it's better to not advertise it at all in the first place. I agree with Daniel that it's better to cripple -sandbox from even being usable as a command-line argument if it isn't going to work, as that's easier to introspect.
On Mon, May 07, 2018 at 01:04:17PM -0500, Eric Blake wrote: >On 05/06/2018 10:32 PM, Yi Min Zhao wrote: > >In the subject line: s/avoid to compile/avoid compiling/ > >> If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains > >s/elevator/elevate/ > >> complied. This would make libvirt set the corresponding capability and > >s/complied/compiled/ > >> then trigger the guest startup fails. So let's wrap the options with >> CONFIG_SECCOMP. >> >> Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> >> --- >> vl.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/vl.c b/vl.c >> index fce1fd12d8..cb07b19c02 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "enable", >> .type = QEMU_OPT_BOOL, >> }, >> +#ifdef CONFIG_SECCOMP >> { >> .name = "obsolete", >> .type = QEMU_OPT_STRING, >> @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { >> .name = "resourcecontrol", >> .type = QEMU_OPT_STRING, >> }, >> +#endif > >The commit message mentions only 'elevateprivileges' (once the typo is >fixed), but you are also crippling 'obsolete', 'spawn', and >'resourcecontrol'. Perhaps the commit message should call that out >better? Or, since libvirt is looking at just 'elevateprivileges', per >this line in libvirt's qemu_capabilities.c: > >src/qemu/qemu_capabilities.c: { "sandbox", "elevateprivileges", >QEMU_CAPS_SECCOMP_BLACKLIST }, > >is it sufficient to just mask out that one option? That would be inconsistent. I picked one option randomly, because they were added at the same time, but compiling out just one out of four is just odd. And with leaving them in even though the functionality is compiled out, libvirt has no way to tell upfront whether it's usable. By that logic, removing the -sandbox option without CONFIG_SECCOMP makes sense, but libvirt already assumes this option is present on all supported QEMUs (>= 1.5.0), so please cc: me on that change if you decide to remove it as well. Jano
diff --git a/vl.c b/vl.c index fce1fd12d8..cb07b19c02 100644 --- a/vl.c +++ b/vl.c @@ -268,6 +268,7 @@ static QemuOptsList qemu_sandbox_opts = { .name = "enable", .type = QEMU_OPT_BOOL, }, +#ifdef CONFIG_SECCOMP { .name = "obsolete", .type = QEMU_OPT_STRING, @@ -284,6 +285,7 @@ static QemuOptsList qemu_sandbox_opts = { .name = "resourcecontrol", .type = QEMU_OPT_STRING, }, +#endif { /* end of list */ } }, };
If CONFIG_SECCOMP is undefined, the option 'elevatorprivileges' remains complied. This would make libvirt set the corresponding capability and then trigger the guest startup fails. So let's wrap the options with CONFIG_SECCOMP. Signed-off-by: Yi Min Zhao <zyimin@linux.ibm.com> --- vl.c | 2 ++ 1 file changed, 2 insertions(+)