src/libvirt_private.syms | 2 ++ src/qemu/qemu_conf.c | 13 +++++++++++-- src/qemu/qemu_driver.c | 21 +++++++++++++-------- src/qemu/qemu_security.h | 2 ++ src/security/security_manager.c | 22 ++++++++++++++++++++++ src/security/security_manager.h | 2 ++ 6 files changed, 52 insertions(+), 10 deletions(-)
We have the stack security driver, which internally manages other security drivers,
just call them "top" and "nested".
We call virSecurityStackPreFork() to lock the top one, and it also locks
and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
it unlocks the top one, but not the nested ones. Thus, if one of the nested
drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we always
surround nested locks with top lock, it is always secure. Because we have got top lock
before fork child libvirtd.
However, it is not always the case in the current code, We discovered this case:
the nested list obtained through the qemuSecurityGetNested() will be locked directly
for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the nested list
is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
The problem stack is as follows:
libvirtd thread1 libvirtd thread2 child libvirtd
| | |
| | |
virsh capabilities qemuProcessLanuch |
| | |
| lock top |
| | |
lock nested | |
| | |
| fork------------------->|(nested lock held by thread1)
| | |
| | |
unlock nested unlock top unlock top
|
|
qemuSecuritySetSocketLabel
|
|
lock nested (deadlock)
v3 changes:
Made modifications based on Michal's comments
- ensured matching qemuSecurityStackLock() and qemuSecurityStackUnlock()
- modify the correct order in libvirt_private.syms
- split the code streamlining part into a separate patch
hongmianquan (2):
security_manager: Ensure top lock is acquired before nested locks
security_manager: Remove redundant qemuSecurityGetNested() call
src/libvirt_private.syms | 2 ++
src/qemu/qemu_conf.c | 13 +++++++++++--
src/qemu/qemu_driver.c | 21 +++++++++++++--------
src/qemu/qemu_security.h | 2 ++
src/security/security_manager.c | 22 ++++++++++++++++++++++
src/security/security_manager.h | 2 ++
6 files changed, 52 insertions(+), 10 deletions(-)
--
2.20.1
On 7/5/24 10:01, hongmianquan wrote:
> We have the stack security driver, which internally manages other security drivers,
> just call them "top" and "nested".
>
> We call virSecurityStackPreFork() to lock the top one, and it also locks
> and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
> it unlocks the top one, but not the nested ones. Thus, if one of the nested
> drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we always
> surround nested locks with top lock, it is always secure. Because we have got top lock
> before fork child libvirtd.
>
> However, it is not always the case in the current code, We discovered this case:
> the nested list obtained through the qemuSecurityGetNested() will be locked directly
> for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the nested list
> is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
>
> The problem stack is as follows:
>
> libvirtd thread1 libvirtd thread2 child libvirtd
> | | |
> | | |
> virsh capabilities qemuProcessLanuch |
> | | |
> | lock top |
> | | |
> lock nested | |
> | | |
> | fork------------------->|(nested lock held by thread1)
> | | |
> | | |
> unlock nested unlock top unlock top
> |
> |
> qemuSecuritySetSocketLabel
> |
> |
> lock nested (deadlock)
>
> v3 changes:
> Made modifications based on Michal's comments
> - ensured matching qemuSecurityStackLock() and qemuSecurityStackUnlock()
> - modify the correct order in libvirt_private.syms
> - split the code streamlining part into a separate patch
>
> hongmianquan (2):
> security_manager: Ensure top lock is acquired before nested locks
> security_manager: Remove redundant qemuSecurityGetNested() call
>
> src/libvirt_private.syms | 2 ++
> src/qemu/qemu_conf.c | 13 +++++++++++--
> src/qemu/qemu_driver.c | 21 +++++++++++++--------
> src/qemu/qemu_security.h | 2 ++
> src/security/security_manager.c | 22 ++++++++++++++++++++++
> src/security/security_manager.h | 2 ++
> 6 files changed, 52 insertions(+), 10 deletions(-)
>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and merged. Congratulations on your first libvirt contribution!
Michal
在 2024/7/9 7:26 下午, Michal Prívozník 写道:
> On 7/5/24 10:01, hongmianquan wrote:
>> We have the stack security driver, which internally manages other security drivers,
>> just call them "top" and "nested".
>>
>> We call virSecurityStackPreFork() to lock the top one, and it also locks
>> and then unlocks the nested drivers prior to fork. Then in qemuSecurityPostFork(),
>> it unlocks the top one, but not the nested ones. Thus, if one of the nested
>> drivers ("dac" or "selinux") is still locked, it will cause a deadlock. If we always
>> surround nested locks with top lock, it is always secure. Because we have got top lock
>> before fork child libvirtd.
>>
>> However, it is not always the case in the current code, We discovered this case:
>> the nested list obtained through the qemuSecurityGetNested() will be locked directly
>> for subsequent use, such as in virQEMUDriverCreateCapabilities(), where the nested list
>> is locked using qemuSecurityGetDOI, but the top one is not locked beforehand.
>>
>> The problem stack is as follows:
>>
>> libvirtd thread1 libvirtd thread2 child libvirtd
>> | | |
>> | | |
>> virsh capabilities qemuProcessLanuch |
>> | | |
>> | lock top |
>> | | |
>> lock nested | |
>> | | |
>> | fork------------------->|(nested lock held by thread1)
>> | | |
>> | | |
>> unlock nested unlock top unlock top
>> |
>> |
>> qemuSecuritySetSocketLabel
>> |
>> |
>> lock nested (deadlock)
>>
>> v3 changes:
>> Made modifications based on Michal's comments
>> - ensured matching qemuSecurityStackLock() and qemuSecurityStackUnlock()
>> - modify the correct order in libvirt_private.syms
>> - split the code streamlining part into a separate patch
>>
>> hongmianquan (2):
>> security_manager: Ensure top lock is acquired before nested locks
>> security_manager: Remove redundant qemuSecurityGetNested() call
>>
>> src/libvirt_private.syms | 2 ++
>> src/qemu/qemu_conf.c | 13 +++++++++++--
>> src/qemu/qemu_driver.c | 21 +++++++++++++--------
>> src/qemu/qemu_security.h | 2 ++
>> src/security/security_manager.c | 22 ++++++++++++++++++++++
>> src/security/security_manager.h | 2 ++
>> 6 files changed, 52 insertions(+), 10 deletions(-)
>>
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
> and merged. Congratulations on your first libvirt contribution!
>
> Michal
>
This is indeed my first time contributing code to the libvirt community,
and there were some imperfections. I will pay more attention in the
future! Thank you for your help and for merging my code!
© 2016 - 2024 Red Hat, Inc.