Patchwork [4/4] Warning messages on net devices hotplug

login
register
mail settings
Submitter Corey Bryant
Date Oct. 24, 2012, 3:39 p.m.
Message ID <50880B96.20802@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/193838/
State New
Headers show

Comments

Corey Bryant - Oct. 24, 2012, 3:39 p.m.
On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>
>>
>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>> allowed.
>>>>
>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>
>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>> is done with a completely different mechanism that involves
>>> file-descriptor passing and does not require executing a helper.
>>>
>>> Paolo
>>>
>>
>> Are you sure net_init_tap() is the right place for this check?
> 
> Yes, assuming there is a global that says whether the seccomp sandbox is
> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
> error(...)" can be enough.
> 
> Paolo
> 

What do you think about this?  It moves the checks into the functions that actually cause execve() to be called, and it only prevents the commands after QEMU is done with initialization in main().

---
Paolo Bonzini - Oct. 24, 2012, 3:45 p.m.
Il 24/10/2012 17:39, Corey Bryant ha scritto:
> 
> 
> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>
>>>
>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>> allowed.
>>>>>
>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>
>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>> is done with a completely different mechanism that involves
>>>> file-descriptor passing and does not require executing a helper.
>>>>
>>>> Paolo
>>>>
>>>
>>> Are you sure net_init_tap() is the right place for this check?
>>
>> Yes, assuming there is a global that says whether the seccomp sandbox is
>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>> error(...)" can be enough.
>>
>> Paolo
>>
> 
> What do you think about this? It moves the checks into the functions
> that actually cause execve() to be called, and it only prevents the
> commands after QEMU is done with initialization in main().

It doesn't do error reporting correctly because these functions do not
get an Error **.  If you change that and use error_setg instead of
error_report, it should be okay.

However, I really think what your testing is not
runstate_is_prelaunch(), it is seccomp_effective().  If you structure
the test like that, it also lets you eliminate the #ifdef (which in
general we prefer to avoid).

Paolo
Corey Bryant - Oct. 24, 2012, 3:56 p.m.
On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>> allowed.
>>>>>>
>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>
>>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>>> is done with a completely different mechanism that involves
>>>>> file-descriptor passing and does not require executing a helper.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Are you sure net_init_tap() is the right place for this check?
>>>
>>> Yes, assuming there is a global that says whether the seccomp sandbox is
>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>> error(...)" can be enough.
>>>
>>> Paolo
>>>
>>
>> What do you think about this? It moves the checks into the functions
>> that actually cause execve() to be called, and it only prevents the
>> commands after QEMU is done with initialization in main().
>
> It doesn't do error reporting correctly because these functions do not
> get an Error **.  If you change that and use error_setg instead of
> error_report, it should be okay.
>
> However, I really think what your testing is not
> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
> the test like that, it also lets you eliminate the #ifdef (which in
> general we prefer to avoid).
>
> Paolo
>

Ok, thanks for the quick feedback!
Corey Bryant - Oct. 24, 2012, 5:30 p.m.
On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>> With the inclusion of the new "double whitelist" seccomp filter, Qemu
>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>> allowed.
>>>>>>
>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>
>>>>> Please check this in net_init_tap instead.  When using libvirt, hotplug
>>>>> is done with a completely different mechanism that involves
>>>>> file-descriptor passing and does not require executing a helper.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Are you sure net_init_tap() is the right place for this check?
>>>
>>> Yes, assuming there is a global that says whether the seccomp sandbox is
>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>> error(...)" can be enough.
>>>
>>> Paolo
>>>
>>
>> What do you think about this? It moves the checks into the functions
>> that actually cause execve() to be called, and it only prevents the
>> commands after QEMU is done with initialization in main().
>
> It doesn't do error reporting correctly because these functions do not
> get an Error **.  If you change that and use error_setg instead of
> error_report, it should be okay.

I just wanted to follow up on a few things..

All of the following functions currently use qerror_report().  I'm 
thinking conversion of these and sub-functions to pass an Error ** 
parameter should be a separate undertaking.

net_init_nic
net_init_slirp
net_init_tap
net_init_socket
net_init_vde
net_init_dump
net_init_bridge
net_init_hubport

>
> However, I really think what your testing is not
> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
> the test like that, it also lets you eliminate the #ifdef (which in
> general we prefer to avoid).

The reason for testing runstate_is_prelaunch() is because seccomp will 
be effective during and after prelaunch.  The only difference is that a 
more restrictive syscall whitelist will be in effect after prelaunch. So 
perhaps the tests can be similar to the following so that we can get rid 
of the preprocessor #ifdef:

if (seccomp_is_effective() && !runstate_is_prelaunch()) {
     error_report("Cannot execute network helper from QEMU monitor "
                  "when -sandbox is in effect");
     return -1;
}
Paolo Bonzini - Oct. 25, 2012, 7:40 a.m.
Il 24/10/2012 19:30, Corey Bryant ha scritto:
> 
> 
> On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
>> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>>
>>>
>>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>>
>>>>>
>>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>>> With the inclusion of the new "double whitelist" seccomp filter,
>>>>>>> Qemu
>>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>>> allowed.
>>>>>>>
>>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>>
>>>>>> Please check this in net_init_tap instead.  When using libvirt,
>>>>>> hotplug
>>>>>> is done with a completely different mechanism that involves
>>>>>> file-descriptor passing and does not require executing a helper.
>>>>>>
>>>>>> Paolo
>>>>>>
>>>>>
>>>>> Are you sure net_init_tap() is the right place for this check?
>>>>
>>>> Yes, assuming there is a global that says whether the seccomp
>>>> sandbox is
>>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>>> error(...)" can be enough.
>>>>
>>>> Paolo
>>>>
>>>
>>> What do you think about this? It moves the checks into the functions
>>> that actually cause execve() to be called, and it only prevents the
>>> commands after QEMU is done with initialization in main().
>>
>> It doesn't do error reporting correctly because these functions do not
>> get an Error **.  If you change that and use error_setg instead of
>> error_report, it should be okay.
> 
> I just wanted to follow up on a few things..
> 
> All of the following functions currently use qerror_report().  I'm
> thinking conversion of these and sub-functions to pass an Error **
> parameter should be a separate undertaking.
> 
> net_init_nic
> net_init_slirp
> net_init_tap
> net_init_socket
> net_init_vde
> net_init_dump
> net_init_bridge
> net_init_hubport

Ok, but it should not be hard considering that the immediate caller of
all these functions (net_client_init1) takes an Error **.  Please
consider this for 1.4 at least.

>> However, I really think what your testing is not
>> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
>> the test like that, it also lets you eliminate the #ifdef (which in
>> general we prefer to avoid).
> 
> The reason for testing runstate_is_prelaunch() is because seccomp will
> be effective during and after prelaunch.  The only difference is that a
> more restrictive syscall whitelist will be in effect after prelaunch. So
> perhaps the tests can be similar to the following so that we can get rid
> of the preprocessor #ifdef:
> 
> if (seccomp_is_effective() && !runstate_is_prelaunch()) {
>     error_report("Cannot execute network helper from QEMU monitor "
>                  "when -sandbox is in effect");
>     return -1;
> }

Then you can make the seccomp query return many levels or flags, like
SECCOMP_SANDBOX_ENABLED | SECCOMP_CAN_EXECVE.

Paolo
Corey Bryant - Oct. 26, 2012, 2:14 p.m.
On 10/25/2012 03:40 AM, Paolo Bonzini wrote:
> Il 24/10/2012 19:30, Corey Bryant ha scritto:
>>
>>
>> On 10/24/2012 11:45 AM, Paolo Bonzini wrote:
>>> Il 24/10/2012 17:39, Corey Bryant ha scritto:
>>>>
>>>>
>>>> On 10/24/2012 11:21 AM, Paolo Bonzini wrote:
>>>>> Il 24/10/2012 16:18, Corey Bryant ha scritto:
>>>>>>
>>>>>>
>>>>>> On 10/18/2012 11:15 AM, Paolo Bonzini wrote:
>>>>>>> Il 17/10/2012 15:15, Eduardo Otubo ha scritto:
>>>>>>>> With the inclusion of the new "double whitelist" seccomp filter,
>>>>>>>> Qemu
>>>>>>>> won't be able to execve() in runtime, thus, no hotplug net devices
>>>>>>>> allowed.
>>>>>>>>
>>>>>>>> Signed-off-by: Eduardo Otubo <otubo@linux.vnet.ibm.com>
>>>>>>>
>>>>>>> Please check this in net_init_tap instead.  When using libvirt,
>>>>>>> hotplug
>>>>>>> is done with a completely different mechanism that involves
>>>>>>> file-descriptor passing and does not require executing a helper.
>>>>>>>
>>>>>>> Paolo
>>>>>>>
>>>>>>
>>>>>> Are you sure net_init_tap() is the right place for this check?
>>>>>
>>>>> Yes, assuming there is a global that says whether the seccomp
>>>>> sandbox is
>>>>> in effect.  Even something like "if (sandbox_active && !tap->has_fd)
>>>>> error(...)" can be enough.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> What do you think about this? It moves the checks into the functions
>>>> that actually cause execve() to be called, and it only prevents the
>>>> commands after QEMU is done with initialization in main().
>>>
>>> It doesn't do error reporting correctly because these functions do not
>>> get an Error **.  If you change that and use error_setg instead of
>>> error_report, it should be okay.
>>
>> I just wanted to follow up on a few things..
>>
>> All of the following functions currently use qerror_report().  I'm
>> thinking conversion of these and sub-functions to pass an Error **
>> parameter should be a separate undertaking.
>>
>> net_init_nic
>> net_init_slirp
>> net_init_tap
>> net_init_socket
>> net_init_vde
>> net_init_dump
>> net_init_bridge
>> net_init_hubport
>
> Ok, but it should not be hard considering that the immediate caller of
> all these functions (net_client_init1) takes an Error **.  Please
> consider this for 1.4 at least.
>
>>> However, I really think what your testing is not
>>> runstate_is_prelaunch(), it is seccomp_effective().  If you structure
>>> the test like that, it also lets you eliminate the #ifdef (which in
>>> general we prefer to avoid).
>>
>> The reason for testing runstate_is_prelaunch() is because seccomp will
>> be effective during and after prelaunch.  The only difference is that a
>> more restrictive syscall whitelist will be in effect after prelaunch. So
>> perhaps the tests can be similar to the following so that we can get rid
>> of the preprocessor #ifdef:
>>
>> if (seccomp_is_effective() && !runstate_is_prelaunch()) {
>>      error_report("Cannot execute network helper from QEMU monitor "
>>                   "when -sandbox is in effect");
>>      return -1;
>> }
>
> Then you can make the seccomp query return many levels or flags, like
> SECCOMP_SANDBOX_ENABLED | SECCOMP_CAN_EXECVE.
>
> Paolo
>

True, we can do that.  I'll take that approach.  Thanks for the suggestion.

Patch

diff --git a/net/tap.c b/net/tap.c
index df89caa..7a8a234 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -352,6 +352,14 @@  static int launch_script(const char *setup_script, const char *ifname, int fd)
     char *args[3];
     char **parg;
 
+#ifdef CONFIG_SECCOMP
+        if (!runstate_is_prelaunch()) {
+            error_report("Cannot execute network script from QEMU monitor "
+                         "when -sandbox is in effect");
+            return -1;
+        }
+#endif
+
     /* try to launch network script */
     pid = fork();
     if (pid == 0) {
@@ -426,6 +434,14 @@  static int net_bridge_run_helper(const char *helper, const char *bridge)
     char **parg;
     int sv[2];
 
+#ifdef CONFIG_SECCOMP
+        if (!runstate_is_prelaunch()) {
+            error_report("Cannot execute network helper from QEMU monitor "
+                         "when -sandbox is in effect");
+            return -1;
+        }
+#endif
+
     sigemptyset(&mask);
     sigaddset(&mask, SIGCHLD);
     sigprocmask(SIG_BLOCK, &mask, &oldmask);
diff --git a/sysemu.h b/sysemu.h
index 0c39a3a..37d8c7d 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -23,6 +23,7 @@  void runstate_init(void);
 bool runstate_check(RunState state);
 void runstate_set(RunState new_state);
 int runstate_is_running(void);
+int runstate_is_prelaunch(void);
 typedef struct vm_change_state_entry VMChangeStateEntry;
 typedef void VMChangeStateHandler(void *opaque, int running, RunState state);
 
diff --git a/vl.c b/vl.c
index c7e88ff..b19b9fa 100644
--- a/vl.c
+++ b/vl.c
@@ -432,6 +432,11 @@  int runstate_is_running(void)
     return runstate_check(RUN_STATE_RUNNING);
 }
 
+int runstate_is_prelaunch(void)
+{
+    return runstate_check(RUN_STATE_PRELAUNCH);
+}
+
 StatusInfo *qmp_query_status(Error **errp)
 {
     StatusInfo *info = g_malloc0(sizeof(*info));