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@xxxxxxxxxx>
and merged. Congratulations on your first libvirt contribution!
Michal
