From nobody Mon Apr 29 08:14:54 2024 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) client-ip=209.132.183.28; envelope-from=libvir-list-bounces@redhat.com; helo=mx1.redhat.com; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass(p=none dis=none) header.from=redhat.com ARC-Seal: i=1; a=rsa-sha256; t=1566477044; cv=none; d=zoho.com; s=zohoarc; b=HFKGpvw6RCgt9A7hL1aqrc48Lnx7lmqsWZEmEj5m1+HofKGTTrVM87YRsjZdo134Q4otlVLQ02qMHKJhThujk0LzQTIllVXr8E7ACR+maRu33f1IkNiU2KPAD+W7r8j7dl0NGoKBWiqpsEYP73RoGzVuFNoYSXd2fn2Jc6yJENk= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com; s=zohoarc; t=1566477044; h=Content-Type:Content-Transfer-Encoding:Date:From:List-Subscribe:List-Post:List-Id:List-Archive:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To:ARC-Authentication-Results; bh=ZzH+Z7V34UKlFHY6n2RMvAbON+47rme1mM80F3zrFns=; b=fayny7RFlOX2FmtVsXD3/2lpyJ1wwIL33F27Hu2qbHlf98ZcJMsvCkeOIF8uD5U+N3IOEkgH2vNQlX9pGYDlnnetfj6Vorbl1MbzA5b39NnCNe4vr+5mQ5ymJTat259QlmGYyNgHHlmdrrOpCFAB/8th+wGMTBHBuV0d+a11Nf4= ARC-Authentication-Results: i=1; mx.zoho.com; spf=pass (zoho.com: domain of redhat.com designates 209.132.183.28 as permitted sender) smtp.mailfrom=libvir-list-bounces@redhat.com; dmarc=pass header.from= (p=none dis=none) header.from= Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.zohomail.com with SMTPS id 1566477044891560.0284513091859; Thu, 22 Aug 2019 05:30:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CCCCC1918641; Thu, 22 Aug 2019 12:30:42 +0000 (UTC) Received: from colo-mx.corp.redhat.com (colo-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D8E465888; Thu, 22 Aug 2019 12:30:40 +0000 (UTC) Received: from lists01.pubmisc.prod.ext.phx2.redhat.com (lists01.pubmisc.prod.ext.phx2.redhat.com [10.5.19.33]) by colo-mx.corp.redhat.com (Postfix) with ESMTP id EC3B124F30; Thu, 22 Aug 2019 12:30:37 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by lists01.pubmisc.prod.ext.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id x7MCUaoQ021572 for ; Thu, 22 Aug 2019 08:30:36 -0400 Received: by smtp.corp.redhat.com (Postfix) id 5D9A38097; Thu, 22 Aug 2019 12:30:36 +0000 (UTC) Received: from moe.brq.redhat.com (unknown [10.43.2.30]) by smtp.corp.redhat.com (Postfix) with ESMTP id DB21F5888 for ; Thu, 22 Aug 2019 12:30:32 +0000 (UTC) From: Michal Privoznik To: libvir-list@redhat.com Date: Thu, 22 Aug 2019 14:30:30 +0200 Message-Id: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-loop: libvir-list@redhat.com Subject: [libvirt] [PATCH] security: Don't increase XATTRs refcounter on failure X-BeenThere: libvir-list@redhat.com X-Mailman-Version: 2.1.12 Precedence: junk List-Id: Development discussions about the libvirt library & tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: quoted-printable Sender: libvir-list-bounces@redhat.com Errors-To: libvir-list-bounces@redhat.com X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.70]); Thu, 22 Aug 2019 12:30:44 +0000 (UTC) Content-Type: text/plain; charset="utf-8" If user has two domains, each have the same disk (configured for RW) but each runs with different seclabel then we deny start of the second domain because in order to do that we would need to relabel the disk but that would cut the first domain off. Even if we did not do that, qemu would fail to start because it would be unable to lock the disk image for the second time. So far, this behaviour is expected. But what is not expected is that we increase the refcounter in XATTRs and leave it like that. What happens is that when the second domain starts, virSecuritySetRememberedLabel() is called, and since there are XATTRs from the first domain it increments the refcounter and returns it (refcounter =3D=3D 2 at this point). Then callers (virSecurityDACSetOwnership() and virSecuritySELinuxSetFileconHelper()) realize that refcounter is greater than 1 and desired seclabel doesn't match the one the disk image already has and an error is produced. But the refcounter is never decremented. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=3D1740024 Signed-off-by: Michal Privoznik Reviewed-by: Martin Kletzander --- src/security/security_dac.c | 19 ++++++++++++++----- src/security/security_selinux.c | 17 +++++++++++------ 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 137daf5d28..b0070f7390 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -754,6 +754,8 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, struct stat sb; int refcount; int rc; + bool rollback =3D false; + int ret =3D -1; =20 if (!path && src && src->path && virStorageSourceIsLocalStorage(src)) @@ -780,16 +782,18 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, } else if (refcount < 0) { return -1; } else if (refcount > 1) { + rollback =3D true; /* Refcount is greater than 1 which means that there * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was incremented in + * XATTRs so decrease it. */ if (sb.st_uid !=3D uid || sb.st_gid !=3D gid) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different DAC user or group on %= s " "which is already in use"), path); - return -1; + goto cleanup; } } } @@ -797,7 +801,13 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'", NULLSTR(src ? src->path : path), (long)uid, (long)gid); =20 - if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0)= { + if (virSecurityDACSetOwnershipInternal(priv, src, path, uid, gid) < 0) + goto cleanup; + + ret =3D 0; + + cleanup: + if (ret < 0 && rollback) { virErrorPtr origerr; =20 virErrorPreserveLast(&origerr); @@ -812,10 +822,9 @@ virSecurityDACSetOwnership(virSecurityManagerPtr mgr, NULLSTR(src ? src->path : path)); =20 virErrorRestore(&origerr); - return -1; } =20 - return 0; + return ret; } =20 =20 diff --git a/src/security/security_selinux.c b/src/security/security_selinu= x.c index ea20373a90..0c6ace75fa 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1334,6 +1334,7 @@ virSecuritySELinuxSetFileconHelper(virSecurityManager= Ptr mgr, security_context_t econ =3D NULL; int refcount; int rc; + bool rollback =3D false; int ret =3D -1; =20 if ((rc =3D virSecuritySELinuxTransactionAppend(path, tcon, @@ -1358,11 +1359,13 @@ virSecuritySELinuxSetFileconHelper(virSecurityManag= erPtr mgr, } else if (refcount < 0) { goto cleanup; } else if (refcount > 1) { + rollback =3D true; /* Refcount is greater than 1 which means that there * is @refcount domains using the @path. Do not * change the label (as it would almost certainly * cause the other domains to lose access to the - * @path). */ + * @path). However, the refcounter was + * incremented in XATTRs so decrease it. */ if (STRNEQ(econ, tcon)) { virReportError(VIR_ERR_OPERATION_INVALID, _("Setting different SELinux label on %= s " @@ -1373,7 +1376,12 @@ virSecuritySELinuxSetFileconHelper(virSecurityManage= rPtr mgr, } } =20 - if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged)= < 0) { + if (virSecuritySELinuxSetFileconImpl(path, tcon, optional, privileged)= < 0) + goto cleanup; + + ret =3D 0; + cleanup: + if (ret < 0 && rollback) { virErrorPtr origerr; =20 virErrorPreserveLast(&origerr); @@ -1388,11 +1396,8 @@ virSecuritySELinuxSetFileconHelper(virSecurityManage= rPtr mgr, path); =20 virErrorRestore(&origerr); - goto cleanup; - } =20 - ret =3D 0; - cleanup: + } freecon(econ); return ret; } --=20 2.21.0 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list