From nobody Mon Sep 16 19:09:20 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) client-ip=8.43.85.245; envelope-from=devel-bounces@lists.libvirt.org; helo=lists.libvirt.org; Authentication-Results: mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=pass(p=reject dis=none) header.from=lists.libvirt.org ARC-Seal: i=1; a=rsa-sha256; t=1719309925; cv=none; d=zohomail.com; s=zohoarc; b=Qm9BnsyBdXyNMcETQVyg9mXjU6SJJXBn7yEBA/FNC7k29+Kyljbihl7SFlr0cUvvEYiMvWP+nKA/wiQ4m6ch0eLrRCS0pGid2KxnpvgYP6WR69SM5E9TlgH1FfUwGoXMI0Y+lChyaJkJoi9fiz7qU6FlLeBHWbeB6nOERv4YOls= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; s=zohoarc; t=1719309925; h=Cc:Cc:Date:Date:From:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:Message-ID:Reply-To:Reply-To:Subject:Subject:To:To:Message-Id; bh=tIrpJr2tKuZlZhxpgvfGi09G1OrLspfxyiCOZiXdKzQ=; b=hbXgLQTI5GlsB2RM6ZQs0Bd9lsi58YjXDTZ+0tQiFPhAUeiAwB3749p4jipi6jOJrqnXA/6R/91Jw4c5zJKarLDgXlY3ZiOjulzfIU8fcawaaQJ/BQg2RiSTByX3G3S46+/fNsE1mExK51MeuVQwKDY8jQxgU6XylWHPUBhh2yM= ARC-Authentication-Results: i=1; mx.zohomail.com; dkim=fail; spf=pass (zohomail.com: domain of lists.libvirt.org designates 8.43.85.245 as permitted sender) smtp.mailfrom=devel-bounces@lists.libvirt.org; dmarc=pass header.from= (p=reject dis=none) Return-Path: Received: from lists.libvirt.org (lists.libvirt.org [8.43.85.245]) by mx.zohomail.com with SMTPS id 1719309925517610.9459761532554; Tue, 25 Jun 2024 03:05:25 -0700 (PDT) Received: by lists.libvirt.org (Postfix, from userid 996) id 2D39D13D8; Tue, 25 Jun 2024 06:05:24 -0400 (EDT) Received: from lists.libvirt.org (localhost [IPv6:::1]) by lists.libvirt.org (Postfix) with ESMTP id ECCAC1382; Tue, 25 Jun 2024 06:04:54 -0400 (EDT) Received: by lists.libvirt.org (Postfix, from userid 996) id DBCBFA46; Tue, 25 Jun 2024 06:03:51 -0400 (EDT) Received: from mail-pl1-f173.google.com (mail-pl1-f173.google.com [209.85.214.173]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by lists.libvirt.org (Postfix) with ESMTPS id 224869E0 for ; Tue, 25 Jun 2024 06:03:50 -0400 (EDT) Received: by mail-pl1-f173.google.com with SMTP id d9443c01a7336-1f9b52ef481so42944645ad.1 for ; Tue, 25 Jun 2024 03:03:49 -0700 (PDT) Received: from n149-064-218.byted.org ([106.38.226.235]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-1f9eb2f04eesm76958495ad.35.2024.06.25.03.03.45 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 25 Jun 2024 03:03:47 -0700 (PDT) X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on lists.libvirt.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_INVALID,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL,SPF_HELO_NONE autolearn=unavailable autolearn_force=no version=3.4.4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1719309828; x=1719914628; darn=lists.libvirt.org; h=message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=tIrpJr2tKuZlZhxpgvfGi09G1OrLspfxyiCOZiXdKzQ=; b=gAQWAvoG/x1yP9sygXrIaAi6btC4hW5WhChaeL0FC07XOQ6VzhlvGHtBYlH6oCoBXU vcUvEt7ZmNg5qluYhgX3h9MVFwdonGjibDUD6aykmDn0RQXInVpPzttDknaHMMKvEC7A eoRmxK/D0wiLTMBZT7JfWo3UUYAb8vrONCqls7ReNX+z5MEzxzYg1KXHpgdJ4TpFmM+S iArUS1xYgAb6IYOo7oTNhU0Njz8+Ytv2UKMkLAVIm7nt3WPaa//Kk/aAdUW8nomXLK97 ZHhjqDLnh2UDEiHeLGu/PTpa1frN+VQNULEfKTN4umtkjCJti942MqTxBsYrHHz8D1O1 CyWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719309828; x=1719914628; h=message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=tIrpJr2tKuZlZhxpgvfGi09G1OrLspfxyiCOZiXdKzQ=; b=lUdAbDUSPPCF5lNO2uyG9WJEtovN5W54mrv1KS/KNaZ5GBOqtkDKiNv5heb6YktYqD pyth2G4CxuaaBesysBblQpvchofRR0ACQ5e9geVhTmsMB7rOeHmEvYKn9oKj9VcyUQii dQjItr6zjozq/Y3CGoyJKUBPRyijywEWWKfS4wWh8OjM10u3x0aPkBHu7c9veCXK7r1p Eu7tMW1rE3JlbMDZY3ExsX0ClJ9n0w5bcz38JU3K7rl4gYlawsRSOK31IIiUlAw2/9jf n+G4F5pJ35Od5BNJF/lNqu0jtTDStgkz942sgL2az00wRtnnY5zP3n9mEMhJ52n4ZwNe 8t/w== X-Gm-Message-State: AOJu0Yxv7lR+iWBvxl5QggYl6jV+Lh/8Tmvp1RpEha2NXciUaZGjdQMC KCIOX2bgxotaOBtJVq12CJU+7c10mu/VUmoMsT+k+ly9YDeJ9ln6Esg1SGEWDKxk72qfiGyusF3 LY5g= X-Google-Smtp-Source: AGHT+IEkxPZ0pVuwPzoJvd74gWqC9qFkK7NLIl4ZrwAgIT+5sG0RKPAO706VO2VAIp5bTkFx1xMMTQ== X-Received: by 2002:a17:902:ee91:b0:1f8:6971:c35d with SMTP id d9443c01a7336-1fa23f9bea7mr59839745ad.68.1719309827583; Tue, 25 Jun 2024 03:03:47 -0700 (PDT) To: devel@lists.libvirt.org Subject: [PATCH v1] security_manager: Ensure top lock is acquired before nested locks Date: Tue, 25 Jun 2024 18:03:40 +0800 Message-Id: <20240625100340.3566029-1-hongmianquan@bytedance.com> X-Mailer: git-send-email 2.11.0 X-MailFrom: hongmianquan@bytedance.com X-Mailman-Rule-Hits: nonmember-moderation X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-config-1; header-match-config-2; header-match-config-3; header-match-devel.lists.libvirt.org-0 Message-ID-Hash: TDSPK5VIRDZMY2J3FOOUWE7WCRHVQ6LS X-Message-ID-Hash: TDSPK5VIRDZMY2J3FOOUWE7WCRHVQ6LS X-Mailman-Approved-At: Tue, 25 Jun 2024 10:04:53 -0400 CC: hongmianquan X-Mailman-Version: 3.2.2 Precedence: list List-Id: Development discussions about the libvirt library & tools Archived-At: List-Archive: List-Help: List-Post: List-Subscribe: List-Unsubscribe: From: hongmianquan via Devel Reply-To: hongmianquan X-ZohoMail-DKIM: fail (Header signature does not verify) X-ZM-MESSAGEID: 1719309926454100001 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" We need to ensure top lock is acquired before nested lock. Otherwise deadlo= ck issues may arise. We have the stack security driver, which internally manag= es other security drivers, we 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 qemuSecurityPost= Fork(), 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. We discovered this case: the nested list obtained through the qemuSecurityG= etNested() will be locked for subsequent use, such as in virQEMUDriverCreateCapabiliti= es(), where the nested list is locked using qemuSecurityGetDOI, but the top one i= s not locked beforehand. The problem stack is as follows: libvirtd thread1 libvirtd thread2 child libvirtd | | | | | | virsh capabilities qemuProcessLanuch | | | | | lock top | | | | lock nested | | | | | | fork------------------->|(held nested l= ock) | | | | | | unlock nested unlock top unlock top | | qemuSecuritySetSocketLabel | | lock nested (deadlock) In this commit, we ensure that the top lock is acquired before the nested l= ock, so during fork, it's not possible for another task to acquire the nested lo= ck. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=3D1303031 Signed-off-by: hongmianquan --- src/libvirt_private.syms | 3 ++- src/qemu/qemu_conf.c | 9 ++++++++- src/qemu/qemu_driver.c | 16 +++++++++------- src/qemu/qemu_security.h | 2 ++ src/security/security_manager.c | 22 ++++++++++++++++++++++ src/security/security_manager.h | 2 ++ 6 files changed, 45 insertions(+), 9 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index bac4a8a366..39cdb90772 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1806,7 +1806,8 @@ virSecurityManagerTransactionAbort; virSecurityManagerTransactionCommit; virSecurityManagerTransactionStart; virSecurityManagerVerify; - +virSecurityManagerStackLock; +virSecurityManagerStackUnlock; =20 # security/security_util.h virSecurityXATTRNamespaceDefined; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 4050a82341..21f0739fd5 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1380,6 +1380,9 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDrive= r *driver) return NULL; } =20 + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + /* access sec drivers and create a sec model for each one */ if (!(sec_managers =3D qemuSecurityGetNested(driver->securityManager))) return NULL; @@ -1402,8 +1405,10 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDriv= er *driver) lbl =3D qemuSecurityGetBaseLabel(sec_managers[i], virtTypes[j]= ); type =3D virDomainVirtTypeToString(virtTypes[j]); if (lbl && - virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0) + virCapabilitiesHostSecModelAddBaseLabel(sm, type, lbl) < 0= ) { + qemuSecurityStackUnlock(driver->securityManager); return NULL; + } } =20 VIR_DEBUG("Initialized caps for security driver \"%s\" with " @@ -1412,6 +1417,8 @@ virCaps *virQEMUDriverCreateCapabilities(virQEMUDrive= r *driver) =20 caps->host.numa =3D virCapabilitiesHostNUMANewHost(); caps->host.cpu =3D virQEMUDriverGetHostCPU(driver); + + qemuSecurityStackUnlock(driver->securityManager); return g_steal_pointer(&caps); } =20 diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fc1704f4fc..c980a0990f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -560,7 +560,6 @@ qemuStateInitialize(bool privileged, bool autostart =3D true; size_t i; const char *defsecmodel =3D NULL; - g_autofree virSecurityManager **sec_managers =3D NULL; g_autoptr(virIdentity) identity =3D virIdentityGetCurrent(); =20 qemu_driver =3D g_new0(virQEMUDriver, 1); @@ -835,11 +834,8 @@ qemuStateInitialize(bool privileged, if (!qemu_driver->qemuCapsCache) goto error; =20 - if (!(sec_managers =3D qemuSecurityGetNested(qemu_driver->securityMana= ger))) - goto error; - - if (sec_managers[0] !=3D NULL) - defsecmodel =3D qemuSecurityGetModel(sec_managers[0]); + if (qemu_driver->securityManager !=3D NULL) + defsecmodel =3D qemuSecurityGetModel(qemu_driver->securityManager); =20 if (!(qemu_driver->xmlopt =3D virQEMUDriverCreateXMLConf(qemu_driver, defsecmodel))) @@ -5663,7 +5659,12 @@ static int qemuDomainGetSecurityLabelList(virDomainP= tr dom, ret =3D 0; } else { int len =3D 0; - virSecurityManager ** mgrs =3D qemuSecurityGetNested(driver->secur= ityManager); + virSecurityManager ** mgrs =3D NULL; + + /* Ensure top lock is acquired before nested locks */ + qemuSecurityStackLock(driver->securityManager); + + mgrs =3D qemuSecurityGetNested(driver->securityManager); if (!mgrs) goto cleanup; =20 @@ -5688,6 +5689,7 @@ static int qemuDomainGetSecurityLabelList(virDomainPt= r dom, } =20 cleanup: + qemuSecurityStackUnlock(driver->securityManager); virDomainObjEndAPI(&vm); return ret; } diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 41da33debc..19fcb3c939 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -151,3 +151,5 @@ int qemuSecurityCommandRun(virQEMUDriver *driver, #define qemuSecuritySetTapFDLabel virSecurityManagerSetTapFDLabel #define qemuSecurityStackAddNested virSecurityManagerStackAddNested #define qemuSecurityVerify virSecurityManagerVerify +#define qemuSecurityStackLock virSecurityManagerStackLock +#define qemuSecurityStackUnlock virSecurityManagerStackUnlock \ No newline at end of file diff --git a/src/security/security_manager.c b/src/security/security_manage= r.c index 24f2f3d3dc..c49c4f708f 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -989,6 +989,28 @@ virSecurityManagerGetNested(virSecurityManager *mgr) return list; } =20 +/* + * Usually called before virSecurityManagerGetNested(). + * We need to ensure locking the stack security manager before + * locking the nested security manager to maintain the correct + * synchronization state. + * It must be followed by a call virSecurityManagerStackUnlock(). + */ +void +virSecurityManagerStackLock(virSecurityManager *mgr) +{ + if (STREQ("stack", mgr->drv->name)) + virObjectLock(mgr); +} + + +void +virSecurityManagerStackUnlock(virSecurityManager *mgr) +{ + if (STREQ("stack", mgr->drv->name)) + virObjectUnlock(mgr); +} + =20 /** * virSecurityManagerDomainSetPathLabel: diff --git a/src/security/security_manager.h b/src/security/security_manage= r.h index a416af3215..bb6d22bc31 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -158,6 +158,8 @@ int virSecurityManagerSetTapFDLabel(virSecurityManager = *mgr, char *virSecurityManagerGetMountOptions(virSecurityManager *mgr, virDomainDef *vm); virSecurityManager ** virSecurityManagerGetNested(virSecurityManager *mgr); +void virSecurityManagerStackLock(virSecurityManager *mgr); +void virSecurityManagerStackUnlock(virSecurityManager *mgr); =20 typedef enum { VIR_SECURITY_DOMAIN_IMAGE_LABEL_BACKING_CHAIN =3D 1 << 0, --=20 2.11.0