[PATCH] qemu_security: Set the label of monitor

Peng Liang posted 1 patch 2 weeks, 4 days ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20210906123606.3970806-1-liangpeng10@huawei.com
src/qemu/qemu_security.c | 6 ++++++
1 file changed, 6 insertions(+)

[PATCH] qemu_security: Set the label of monitor

Posted by Peng Liang 2 weeks, 4 days ago
Signed-off-by: Peng Liang <liangpeng10@huawei.com>
---
 src/qemu/qemu_security.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 19d957dd4b96..96755a62bd2c 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
                                       priv->chardevStdioLogd,
                                       migrated) < 0)
         goto cleanup;
+    if (priv->monConfig &&
+        virSecurityManagerSetChardevLabel(driver->securityManager,
+                                          vm->def,
+                                          priv->monConfig,
+                                          priv->chardevStdioLogd) < 0)
+        goto cleanup;
 
     if (virSecurityManagerTransactionCommit(driver->securityManager,
                                             pid, priv->rememberOwner) < 0)
-- 
2.31.1


Re: [PATCH] qemu_security: Set the label of monitor

Posted by Michal Prívozník 2 weeks, 4 days ago
On 9/6/21 2:36 PM, Peng Liang wrote:
> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
> ---
>  src/qemu/qemu_security.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
> index 19d957dd4b96..96755a62bd2c 100644
> --- a/src/qemu/qemu_security.c
> +++ b/src/qemu/qemu_security.c
> @@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
>                                        priv->chardevStdioLogd,
>                                        migrated) < 0)
>          goto cleanup;
> +    if (priv->monConfig &&
> +        virSecurityManagerSetChardevLabel(driver->securityManager,
> +                                          vm->def,
> +                                          priv->monConfig,
> +                                          priv->chardevStdioLogd) < 0)
> +        goto cleanup;
>  
>      if (virSecurityManagerTransactionCommit(driver->securityManager,
>                                              pid, priv->rememberOwner) < 0)
> 

Is there a specific bug that you are trying to solve? If so then it
should be recorded in the commit message. But anyway - libvirt shouldn't
have any difficulties connecting to the socket. The "setXXXLabel"
functions are meant to grant access to QEMU and in the case of monitor
it's actually QEMU who creates the socket. Having said that, whatever
this patch tries to solve doesn't feel right.

Michal

Re: [PATCH] qemu_security: Set the label of monitor

Posted by Peng Liang 2 weeks, 4 days ago
On 9/6/2021 9:21 PM, Michal Prívozník wrote:
> On 9/6/21 2:36 PM, Peng Liang wrote:
>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>> ---
>>  src/qemu/qemu_security.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>> index 19d957dd4b96..96755a62bd2c 100644
>> --- a/src/qemu/qemu_security.c
>> +++ b/src/qemu/qemu_security.c
>> @@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
>>                                        priv->chardevStdioLogd,
>>                                        migrated) < 0)
>>          goto cleanup;
>> +    if (priv->monConfig &&
>> +        virSecurityManagerSetChardevLabel(driver->securityManager,
>> +                                          vm->def,
>> +                                          priv->monConfig,
>> +                                          priv->chardevStdioLogd) < 0)
>> +        goto cleanup;
>>  
>>      if (virSecurityManagerTransactionCommit(driver->securityManager,
>>                                              pid, priv->rememberOwner) < 0)
>>
> 
> Is there a specific bug that you are trying to solve?Not a functional bug.  Just when using qemu to run QEMU process, I found
that the socket of monitor will not be changed to qemu:qemu while other
sockets (e.g. the socket of qemu agent) will.

> If so then it
> should be recorded in the commit message. But anyway - libvirt shouldn't
> have any difficulties connecting to the socket. The "setXXXLabel"
> functions are meant to grant access to QEMU and in the case of monitor
> it's actually QEMU who creates the socket.If QEMU support to accept fd for chardev, then libvirt (not QEMU) will
create and pass the fd to QEMU.

> Having said that, whatever
> this patch tries to solve doesn't feel right.
> 
> Michal
> 
> .
> 

Thanks,
Peng


Re: [PATCH] qemu_security: Set the label of monitor

Posted by Michal Prívozník 2 weeks, 4 days ago
On 9/6/21 4:33 PM, Peng Liang wrote:
> On 9/6/2021 9:21 PM, Michal Prívozník wrote:
>> On 9/6/21 2:36 PM, Peng Liang wrote:
>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>> ---
>>>  src/qemu/qemu_security.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>>> index 19d957dd4b96..96755a62bd2c 100644
>>> --- a/src/qemu/qemu_security.c
>>> +++ b/src/qemu/qemu_security.c
>>> @@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
>>>                                        priv->chardevStdioLogd,
>>>                                        migrated) < 0)
>>>          goto cleanup;
>>> +    if (priv->monConfig &&
>>> +        virSecurityManagerSetChardevLabel(driver->securityManager,
>>> +                                          vm->def,
>>> +                                          priv->monConfig,
>>> +                                          priv->chardevStdioLogd) < 0)
>>> +        goto cleanup;
>>>  
>>>      if (virSecurityManagerTransactionCommit(driver->securityManager,
>>>                                              pid, priv->rememberOwner) < 0)
>>>
>>
>> Is there a specific bug that you are trying to solve?Not a functional bug.  Just when using qemu to run QEMU process, I found
> that the socket of monitor will not be changed to qemu:qemu while other
> sockets (e.g. the socket of qemu agent) will.
> 
>> If so then it
>> should be recorded in the commit message. But anyway - libvirt shouldn't
>> have any difficulties connecting to the socket. The "setXXXLabel"
>> functions are meant to grant access to QEMU and in the case of monitor
>> it's actually QEMU who creates the socket.If QEMU support to accept fd for chardev, then libvirt (not QEMU) will
> create and pass the fd to QEMU.

So the only QEMU version that we currently support and doesn't have FD
passing is 2.11.0. The FD passing was implemented in 2.12.0.
I don't think it's worth the trouble and also I think this way it's a
bit safer since a regular user can't go beyond libvirt's back and
connect to the monitor directly. While we do have qemu-monitor-command
it is going to log the arguments so sysadmin at least knows what
commands were executed.

At any rate, this is not the correct place. That would be where the
socket is created (qemuBuildChrChardevStr) but then again, I don't think
it's worth the trouble.

Michal

Re: [PATCH] qemu_security: Set the label of monitor

Posted by Peng Liang 2 weeks, 3 days ago
On 9/6/2021 10:52 PM, Michal Prívozník wrote:
> On 9/6/21 4:33 PM, Peng Liang wrote:
>> On 9/6/2021 9:21 PM, Michal Prívozník wrote:
>>> On 9/6/21 2:36 PM, Peng Liang wrote:
>>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>>> ---
>>>>  src/qemu/qemu_security.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
>>>> index 19d957dd4b96..96755a62bd2c 100644
>>>> --- a/src/qemu/qemu_security.c
>>>> +++ b/src/qemu/qemu_security.c
>>>> @@ -52,6 +52,12 @@ qemuSecuritySetAllLabel(virQEMUDriver *driver,
>>>>                                        priv->chardevStdioLogd,
>>>>                                        migrated) < 0)
>>>>          goto cleanup;
>>>> +    if (priv->monConfig &&
>>>> +        virSecurityManagerSetChardevLabel(driver->securityManager,
>>>> +                                          vm->def,
>>>> +                                          priv->monConfig,
>>>> +                                          priv->chardevStdioLogd) < 0)
>>>> +        goto cleanup;
>>>>  
>>>>      if (virSecurityManagerTransactionCommit(driver->securityManager,
>>>>                                              pid, priv->rememberOwner) < 0)
>>>>
>>>
>>> Is there a specific bug that you are trying to solve?Not a functional bug.  Just when using qemu to run QEMU process, I found
>> that the socket of monitor will not be changed to qemu:qemu while other
>> sockets (e.g. the socket of qemu agent) will.
>>
>>> If so then it
>>> should be recorded in the commit message. But anyway - libvirt shouldn't
>>> have any difficulties connecting to the socket. The "setXXXLabel"
>>> functions are meant to grant access to QEMU and in the case of monitor
>>> it's actually QEMU who creates the socket.If QEMU support to accept fd for chardev, then libvirt (not QEMU) will
>> create and pass the fd to QEMU.
> 
> So the only QEMU version that we currently support and doesn't have FD
> passing is 2.11.0. The FD passing was implemented in 2.12.0.
> I don't think it's worth the trouble and also I think this way it's a
> bit safer since a regular user can't go beyond libvirt's back and
> connect to the monitor directly. While we do have qemu-monitor-command
> it is going to log the arguments so sysadmin at least knows what
> commands were executed.
> 
> At any rate, this is not the correct place. That would be where the
> socket is created (qemuBuildChrChardevStr) but then again, I don't think
> it's worth the trouble.
> 
> Michal
> 
> .
> 

OK, I'll drop this patch.

Thanks,
Peng