5:51 p.m.
With the apparmor security driver enabled, qemu instances fail
to start
# grep ^security_driver /etc/libvirt/qemu.conf
security_driver = "apparmor"
# virsh start test-kvm
error: Failed to start domain test-kvm
error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated
by virDomainDefGetSecurityLabelDef(), so remove the check for a
NULL model when verifying if a label is already defined for the
instance.
Checking for a NULL model and populating it later in
AppArmorGenSecurityLabel() has been left in the code to be
consistent with virSecuritySELinuxGenSecurityLabel().
---
src/security/security_apparmor.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index ddc1fe4..2e6a57f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -436,8 +436,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
return rc;
}
- if ((secdef->label) ||
- (secdef->model) || (secdef->imagelabel)) {
+ if (secdef->label || secdef->imagelabel) {
virReportError(VIR_ERR_INTERNAL_ERROR,
"%s",
_("security label already defined for VM"));
@@ -461,8 +460,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
goto err;
}
- secdef->model = strdup(SECURITY_APPARMOR_NAME);
- if (!secdef->model) {
+ if (!secdef->model && !(secdef->model =
strdup(SECURITY_APPARMOR_NAME))) {
virReportOOMError();
goto err;
}
--
1.8.0.1
4:59 p.m.
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
With the apparmor security driver enabled, qemu instances fail
to start
# grep ^security_driver /etc/libvirt/qemu.conf
security_driver = "apparmor"
# virsh start test-kvm
error: Failed to start domain test-kvm
error: internal error security label already defined for VM
The model field of virSecurityLabelDef object is always populated
by virDomainDefGetSecurityLabelDef(), so remove the check for a
NULL model when verifying if a label is already defined for the
instance.
Checking for a NULL model and populating it later in
AppArmorGenSecurityLabel() has been left in the code to be
consistent with virSecuritySELinuxGenSecurityLabel().
---
src/security/security_apparmor.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
ACK; and safe for 1.0.3.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
6:37 p.m.
Eric Blake wrote:
On 02/27/2013 04:51 PM, Jim Fehlig wrote:
> With the apparmor security driver enabled, qemu instances fail
> to start
>
> # grep ^security_driver /etc/libvirt/qemu.conf
> security_driver = "apparmor"
> # virsh start test-kvm
> error: Failed to start domain test-kvm
> error: internal error security label already defined for VM
>
> The model field of virSecurityLabelDef object is always populated
> by virDomainDefGetSecurityLabelDef(), so remove the check for a
> NULL model when verifying if a label is already defined for the
> instance.
>
> Checking for a NULL model and populating it later in
> AppArmorGenSecurityLabel() has been left in the code to be
> consistent with virSecuritySELinuxGenSecurityLabel().
> ---
> src/security/security_apparmor.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
ACK; and safe for 1.0.3.
Thanks, pushed now.
11:53 p.m.
On 03/01/2013 08:37 AM, Jim Fehlig wrote:
Eric Blake wrote:
> On 02/27/2013 04:51 PM, Jim Fehlig wrote:
>
>> With the apparmor security driver enabled, qemu instances fail
>> to start
>>
>> # grep ^security_driver /etc/libvirt/qemu.conf
>> security_driver = "apparmor"
>> # virsh start test-kvm
>> error: Failed to start domain test-kvm
>> error: internal error security label already defined for VM
>>
>> The model field of virSecurityLabelDef object is always populated
>> by virDomainDefGetSecurityLabelDef(), so remove the check for a
>> NULL model when verifying if a label is already defined for the
>> instance.
>>
>> Checking for a NULL model and populating it later in
>> AppArmorGenSecurityLabel() has been left in the code to be
>> consistent with virSecuritySELinuxGenSecurityLabel().
>> ---
>> src/security/security_apparmor.c | 6 ++----
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
> ACK; and safe for 1.0.3.
>
Thanks, pushed now.
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Hi Jim
In selinux, libvirt added a label for tapfd.
Do you think this patch makes sense for apparmor?
https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Gunannan
10:41 a.m.
Guannan Ren wrote:
On 03/01/2013 08:37 AM, Jim Fehlig wrote:
> Eric Blake wrote:
>> On 02/27/2013 04:51 PM, Jim Fehlig wrote:
>>
>>> With the apparmor security driver enabled, qemu instances fail
>>> to start
>>>
>>> # grep ^security_driver /etc/libvirt/qemu.conf
>>> security_driver = "apparmor"
>>> # virsh start test-kvm
>>> error: Failed to start domain test-kvm
>>> error: internal error security label already defined for VM
>>>
>>> The model field of virSecurityLabelDef object is always populated
>>> by virDomainDefGetSecurityLabelDef(), so remove the check for a
>>> NULL model when verifying if a label is already defined for the
>>> instance.
>>>
>>> Checking for a NULL model and populating it later in
>>> AppArmorGenSecurityLabel() has been left in the code to be
>>> consistent with virSecuritySELinuxGenSecurityLabel().
>>> ---
>>> src/security/security_apparmor.c | 6 ++----
>>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>> ACK; and safe for 1.0.3.
>>
> Thanks, pushed now.
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
Hi Jim
In selinux, libvirt added a label for tapfd.
Do you think this patch makes sense for apparmor?
https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Hi Gunannan,
Apologies for missing your initial post of that series. I see that you
fixed this exact bug in 2/3 :(.
I think 3/3 does make sense for apparmor, but I'm not sure about using
AppArmorSetImageFDLabel() as a common function. It returns if
secdef->imagelabel == NULL, which would be incorrect if labeling a tap
fd right?
I promise not to miss the patch if you respin it :).
Regards,
Jim
3:14 a.m.
On 03/02/2013 12:41 AM, Jim Fehlig wrote:
Guannan Ren wrote:
> Hi Jim
>
> In selinux, libvirt added a label for tapfd.
> Do you think this patch makes sense for apparmor?
> https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
Hi Gunannan,
Apologies for missing your initial post of that series. I see that you
fixed this exact bug in 2/3 :(.
I think 3/3 does make sense for apparmor, but I'm not sure about using
AppArmorSetImageFDLabel() as a common function. It returns if
secdef->imagelabel == NULL, which would be incorrect if labeling a tap
fd right?
I promise not to miss the patch if you respin it :).
Regards,
Jim
Nothing to apologize, I really don't know much about apparmor. The
tapfd I mean here
is not used by libvirt deamon, it is a tapfd created on particular
guest which is using macvtap driver
to attach virtual NIC to a given physical interface.
From the code, the secdef->imagelabel have the same value as
secdef->label
which is libvirt-{uuid} file in /etc/apparmor.d/libvirt folder.
If it is null, that means the guest will not
be confined by apparmor, so is this tapfd, I think this is fine.
If you think it is reasonable, I will rebase that patch and send a v2.
Guannan
5:23 p.m.
Guannan Ren wrote:
On 03/02/2013 12:41 AM, Jim Fehlig wrote:
> Guannan Ren wrote:
>
>> Hi Jim
>>
>> In selinux, libvirt added a label for tapfd.
>> Do you think this patch makes sense for apparmor?
>> https://www.redhat.com/archives/libvir-list/2012-October/msg01461.html
> Hi Gunannan,
>
> Apologies for missing your initial post of that series. I see that you
> fixed this exact bug in 2/3 :(.
>
> I think 3/3 does make sense for apparmor, but I'm not sure about using
> AppArmorSetImageFDLabel() as a common function. It returns if
> secdef->imagelabel == NULL, which would be incorrect if labeling a tap
> fd right?
>
> I promise not to miss the patch if you respin it :).
>
> Regards,
> Jim
>
Nothing to apologize, I really don't know much about apparmor.
The tapfd I mean here
is not used by libvirt deamon, it is a tapfd created on
particular guest which is using macvtap driver
to attach virtual NIC to a given physical interface.
From the code, the secdef->imagelabel have the same value as
secdef->label
which is libvirt-{uuid} file in /etc/apparmor.d/libvirt folder.
If it is null, that means the guest will not
be confined by apparmor, so is this tapfd, I think this is fine.
Yes, agreed.
If you think it is reasonable, I will rebase that patch and send
a v2.
Yep, I think it is reasonable and necessary. I finally got around to
testing your patch and it is indeed needed when using macvtap with
apparmor-confined guests.
Thanks for looking into this!
Regards,
Jim
10:16 a.m.
New subject: [libvirt] [PATCH v2] apparmor: use AppArmorSetFDLabel for both imageFD and tapFD
Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could
be used as a common function for *ALL* fd relabelling in Linux.
In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773
Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files
The last line is for the tapfd relabelling.
# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.
"/var/log/libvirt/**/rhel6qcow2.log" w,
"/var/lib/libvirt/**/rhel6qcow2.monitor" rw,
"/var/run/libvirt/**/rhel6qcow2.pid" rwk,
"/run/libvirt/**/rhel6qcow2.pid" rwk,
"/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
"/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
"/var/lib/libvirt/images/rhel6u3qcow2.img" rw,
"/dev/tap45" rw,
---
src/security/security_apparmor.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 2e6a57f..9dd8d74 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -884,9 +884,9 @@ AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
}
static int
-AppArmorSetImageFDLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- int fd)
+AppArmorSetFDLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ int fd)
{
int rc = -1;
char *proc = NULL;
@@ -915,16 +915,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr,
return reload_profile(mgr, def, fd_path, true);
}
-/* TODO need code here */
-static int
-AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
- virDomainDefPtr def ATTRIBUTE_UNUSED,
- int fd ATTRIBUTE_UNUSED)
-{
- return 0;
-}
-
-
static char *
AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED)
@@ -975,8 +965,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
.domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel,
- .domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel,
- .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel,
+ .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
+ .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
.domainGetSecurityMountOptions = AppArmorGetMountOptions,
};
--
1.7.11.2
3:21 p.m.
New subject: [libvirt] [PATCH v2] apparmor: use AppArmorSetFDLabel for both imageFD and tapFD
Guannan Ren wrote:
Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could
be used as a common function for *ALL* fd relabelling in Linux.
In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773
Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files
The last line is for the tapfd relabelling.
# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.
"/var/log/libvirt/**/rhel6qcow2.log" w,
"/var/lib/libvirt/**/rhel6qcow2.monitor" rw,
"/var/run/libvirt/**/rhel6qcow2.pid" rwk,
"/run/libvirt/**/rhel6qcow2.pid" rwk,
"/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
"/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
"/var/lib/libvirt/images/rhel6u3qcow2.img" rw,
"/dev/tap45" rw,
---
src/security/security_apparmor.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
ACK.
diff --git a/src/security/security_apparmor.c
b/src/security/security_apparmor.c
index 2e6a57f..9dd8d74 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -884,9 +884,9 @@ AppArmorRestoreSavedStateLabel(virSecurityManagerPtr mgr,
}
static int
-AppArmorSetImageFDLabel(virSecurityManagerPtr mgr,
- virDomainDefPtr def,
- int fd)
+AppArmorSetFDLabel(virSecurityManagerPtr mgr,
+ virDomainDefPtr def,
+ int fd)
{
int rc = -1;
char *proc = NULL;
@@ -915,16 +915,6 @@ AppArmorSetImageFDLabel(virSecurityManagerPtr mgr,
return reload_profile(mgr, def, fd_path, true);
}
-/* TODO need code here */
-static int
-AppArmorSetTapFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
- virDomainDefPtr def ATTRIBUTE_UNUSED,
- int fd ATTRIBUTE_UNUSED)
-{
- return 0;
-}
-
-
static char *
AppArmorGetMountOptions(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefPtr vm ATTRIBUTE_UNUSED)
@@ -975,8 +965,8 @@ virSecurityDriver virAppArmorSecurityDriver = {
.domainSetSavedStateLabel = AppArmorSetSavedStateLabel,
.domainRestoreSavedStateLabel = AppArmorRestoreSavedStateLabel,
- .domainSetSecurityImageFDLabel = AppArmorSetImageFDLabel,
- .domainSetSecurityTapFDLabel = AppArmorSetTapFDLabel,
+ .domainSetSecurityImageFDLabel = AppArmorSetFDLabel,
+ .domainSetSecurityTapFDLabel = AppArmorSetFDLabel,
.domainGetSecurityMountOptions = AppArmorGetMountOptions,
};
8:57 p.m.
New subject: [libvirt] [PATCH v2] apparmor: use AppArmorSetFDLabel for both imageFD and tapFD
On 03/08/2013 05:21 AM, Jim Fehlig wrote:
Guannan Ren wrote:
> Rename AppArmorSetImageFDLabel to AppArmorSetFDLabel which could
> be used as a common function for *ALL* fd relabelling in Linux.
>
> In apparmor profile for specific vm with uuid cdbebdfa-1d6d-65c3-be0f-fd74b978a773
> Path: /etc/apparmor.d/libvirt/libvirt-cdbebdfa-1d6d-65c3-be0f-fd74b978a773.files
> The last line is for the tapfd relabelling.
>
> # DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.
> "/var/log/libvirt/**/rhel6qcow2.log" w,
> "/var/lib/libvirt/**/rhel6qcow2.monitor" rw,
> "/var/run/libvirt/**/rhel6qcow2.pid" rwk,
> "/run/libvirt/**/rhel6qcow2.pid" rwk,
> "/var/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
> "/run/libvirt/**/*.tunnelmigrate.dest.rhel6qcow2" rw,
> "/var/lib/libvirt/images/rhel6u3qcow2.img" rw,
> "/dev/tap45" rw,
> ---
> src/security/security_apparmor.c | 20 +++++---------------
> 1 file changed, 5 insertions(+), 15 deletions(-)
>
ACK.
Thanks, pushed.
Guannan
4276
days inactive
4285
days old
9 comments
3 participants
participants (3)
-
Eric Blake -
Guannan Ren -
Jim Fehlig