11:32 a.m.
Patch1 is a small improvement. Patch2 fixes a bug that is triggered
when the apparmor security driver is enabled and domain config
specifies a different security driver. E.g.
# virsh dumpxml test | grep seclabel
<seclabel type='none' model='none'/>
# virsh start test
error: Failed to start domain test
error: An error occurred, but the cause is unknown
Jim Fehlig (2):
apparmor: don't overwrite error from reload_profile
apparmor: don't fail on non-apparmor <seclabel>
src/security/security_apparmor.c | 72 ++++++++++------------------------------
1 file changed, 18 insertions(+), 54 deletions(-)
--
2.9.2
11:32 a.m.
New subject: [libvirt] [PATCH 1/2] apparmor: don't overwrite error from reload_profile
Like other callers of reload_profile, don't overwrite errors in
AppArmorSetSecurityHostdevLabelHelper.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/security/security_apparmor.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index 2c33abb..ad50b08 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -322,19 +322,7 @@ AppArmorSetSecurityHostdevLabelHelper(const char *file, void
*opaque)
struct SDPDOP *ptr = opaque;
virDomainDefPtr def = ptr->def;
- if (reload_profile(ptr->mgr, def, file, true) < 0) {
- virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
- def, SECURITY_APPARMOR_NAME);
- if (!secdef) {
- virReportOOMError();
- return -1;
- }
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot update AppArmor profile \'%s\'"),
- secdef->imagelabel);
- return -1;
- }
- return 0;
+ return reload_profile(ptr->mgr, def, file, true);
}
static int
--
2.9.2
9:45 a.m.
New subject: [libvirt] [PATCH 1/2] apparmor: don't overwrite error from reload_profile
On 02/03/2017 06:32 PM, Jim Fehlig wrote:
Like other callers of reload_profile, don't overwrite errors in
AppArmorSetSecurityHostdevLabelHelper.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/security/security_apparmor.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
ACK
Michal
11:32 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
If the apparmor security driver is loaded/enabled and domain config
contains a <seclabel> element whose type attribute is not 'apparmor',
starting the domain fails when attempting to label resources such
as tap FDs.
Many of the apparmor driver entry points attempt to retrieve the
apparmor security label from the domain def, returning failure if
not found. Functions such as AppArmorSetFDLabel fail even though
domain config contains an explicit 'none' secuirty driver, e.g.
<seclabel type='none' model='none'/>
Change the entry points to succeed if the domain config <seclabel>
is not apparmor. This matches the behavior of the selinux driver.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/security/security_apparmor.c | 58 ++++++++++++----------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index ad50b08..f871e60 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -289,10 +289,7 @@ reload_profile(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- return rc;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
if ((profile_name = get_profile_name(def)) == NULL)
@@ -435,7 +432,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
SECURITY_APPARMOR_NAME);
if (!secdef)
- return -1;
+ return 0;
if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) ||
(secdef->type == VIR_DOMAIN_SECLABEL_NONE))
@@ -495,10 +492,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
/* Reload the profile if stdin_path is specified. Note that
@@ -559,12 +553,11 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- VIR_FREE(secdef->imagelabel);
+ if (secdef) {
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);\
+ }
return 0;
}
@@ -580,7 +573,7 @@ AppArmorRestoreSecurityAllLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
if (!secdef)
- return -1;
+ return 0;
if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) {
if ((rc = remove_profile(secdef->label)) != 0) {
@@ -604,10 +597,7 @@ AppArmorSetSecurityProcessLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
virSecurityLabelDefPtr secdef =
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- if (secdef->label == NULL)
+ if (!secdef || !secdef->label)
return 0;
if ((profile_name = get_profile_name(def)) == NULL)
@@ -653,10 +643,7 @@ AppArmorSetSecurityChildProcessLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
virSecurityLabelDefPtr secdef =
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- goto cleanup;
-
- if (secdef->label == NULL)
+ if (!secdef || !secdef->label)
return 0;
if (STRNEQ(SECURITY_APPARMOR_NAME, secdef->model)) {
@@ -738,10 +725,8 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
if (!src->path || !virStorageSourceIsLocalStorage(src))
return 0;
- if (!(secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME)))
- return -1;
-
- if (!secdef->relabel)
+ secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
+ if (!secdef || !secdef->relabel)
return 0;
if (secdef->imagelabel) {
@@ -792,7 +777,7 @@ AppArmorSecurityVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
if (!secdef)
- return -1;
+ return 0;
if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) {
if (use_apparmor() < 0 || profile_status(secdef->label, 0) < 0) {
@@ -829,10 +814,7 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
virDomainHostdevSubsysSCSIVHostPtr hostsrc = &dev->source.subsys.u.scsi_host;
- if (!secdef)
- return -1;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
if (dev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
@@ -940,10 +922,7 @@ AppArmorRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef =
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
return reload_profile(mgr, def, NULL, false);
@@ -978,10 +957,7 @@ AppArmorSetFDLabel(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef =
virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- if (secdef->imagelabel == NULL)
+ if (!secdef || !secdef->imagelabel)
return 0;
if (virAsprintf(&proc, "/proc/self/fd/%d", fd) == -1)
--
2.9.2
5:41 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
On Fri, Feb 03, 2017 at 10:32:12AM -0700, Jim Fehlig wrote:
If the apparmor security driver is loaded/enabled and domain config
contains a <seclabel> element whose type attribute is not 'apparmor',
starting the domain fails when attempting to label resources such
as tap FDs.
Many of the apparmor driver entry points attempt to retrieve the
apparmor security label from the domain def, returning failure if
not found. Functions such as AppArmorSetFDLabel fail even though
domain config contains an explicit 'none' secuirty driver, e.g.
<seclabel type='none' model='none'/>
Change the entry points to succeed if the domain config <seclabel>
is not apparmor. This matches the behavior of the selinux driver.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/security/security_apparmor.c | 58 ++++++++++++----------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index ad50b08..f871e60 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -289,10 +289,7 @@ reload_profile(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
def, SECURITY_APPARMOR_NAME);
- if (!secdef)
- return rc;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
if ((profile_name = get_profile_name(def)) == NULL)
@@ -435,7 +432,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
SECURITY_APPARMOR_NAME);
if (!secdef)
- return -1;
+ return 0;
if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) ||
(secdef->type == VIR_DOMAIN_SECLABEL_NONE))
@@ -495,10 +492,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- if (!secdef->relabel)
+ if (!secdef || !secdef->relabel)
return 0;
/* Reload the profile if stdin_path is specified. Note that
@@ -559,12 +553,11 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
{
virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
SECURITY_APPARMOR_NAME);
- if (!secdef)
- return -1;
-
- VIR_FREE(secdef->model);
- VIR_FREE(secdef->label);
- VIR_FREE(secdef->imagelabel);
+ if (secdef) {
+ VIR_FREE(secdef->model);
+ VIR_FREE(secdef->label);
+ VIR_FREE(secdef->imagelabel);\
The trailing slash can be dropped here. The rest of th code looks good
to me.
-- Guido
10:30 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
Guido Günther wrote:
On Fri, Feb 03, 2017 at 10:32:12AM -0700, Jim Fehlig wrote:
> If the apparmor security driver is loaded/enabled and domain config
> contains a <seclabel> element whose type attribute is not 'apparmor',
> starting the domain fails when attempting to label resources such
> as tap FDs.
>
> Many of the apparmor driver entry points attempt to retrieve the
> apparmor security label from the domain def, returning failure if
> not found. Functions such as AppArmorSetFDLabel fail even though
> domain config contains an explicit 'none' secuirty driver, e.g.
>
> <seclabel type='none' model='none'/>
>
> Change the entry points to succeed if the domain config <seclabel>
> is not apparmor. This matches the behavior of the selinux driver.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> src/security/security_apparmor.c | 58 ++++++++++++----------------------------
> 1 file changed, 17 insertions(+), 41 deletions(-)
>
> diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> index ad50b08..f871e60 100644
> --- a/src/security/security_apparmor.c
> +++ b/src/security/security_apparmor.c
> @@ -289,10 +289,7 @@ reload_profile(virSecurityManagerPtr mgr,
> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
> def, SECURITY_APPARMOR_NAME);
>
> - if (!secdef)
> - return rc;
> -
> - if (!secdef->relabel)
> + if (!secdef || !secdef->relabel)
> return 0;
>
> if ((profile_name = get_profile_name(def)) == NULL)
> @@ -435,7 +432,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
> SECURITY_APPARMOR_NAME);
>
> if (!secdef)
> - return -1;
> + return 0;
>
> if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) ||
> (secdef->type == VIR_DOMAIN_SECLABEL_NONE))
> @@ -495,10 +492,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
> {
> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> - if (!secdef)
> - return -1;
> -
> - if (!secdef->relabel)
> + if (!secdef || !secdef->relabel)
> return 0;
>
> /* Reload the profile if stdin_path is specified. Note that
> @@ -559,12 +553,11 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
> {
> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
> SECURITY_APPARMOR_NAME);
> - if (!secdef)
> - return -1;
> -
> - VIR_FREE(secdef->model);
> - VIR_FREE(secdef->label);
> - VIR_FREE(secdef->imagelabel);
> + if (secdef) {
> + VIR_FREE(secdef->model);
> + VIR_FREE(secdef->label);
> + VIR_FREE(secdef->imagelabel);\
The trailing slash can be dropped here. The rest of th code looks good
to me.
Opps, thanks for catching that. I've removed it in my local branch.
Do we need someone else with knowledge of the apparmor driver to review and ACK
these changes?
Regards,
Jim
2:58 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
Hi, Jim,
On Thu, Feb 09, 2017 at 09:30:16AM -0700, Jim Fehlig wrote:
Guido Günther wrote:
> On Fri, Feb 03, 2017 at 10:32:12AM -0700, Jim Fehlig wrote:
>> If the apparmor security driver is loaded/enabled and domain config
>> contains a <seclabel> element whose type attribute is not
'apparmor',
>> starting the domain fails when attempting to label resources such
>> as tap FDs.
>>
>> Many of the apparmor driver entry points attempt to retrieve the
>> apparmor security label from the domain def, returning failure if
>> not found. Functions such as AppArmorSetFDLabel fail even though
>> domain config contains an explicit 'none' secuirty driver, e.g.
>>
>> <seclabel type='none' model='none'/>
>>
>> Change the entry points to succeed if the domain config <seclabel>
>> is not apparmor. This matches the behavior of the selinux driver.
>>
>> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
>> ---
>> src/security/security_apparmor.c | 58 ++++++++++++----------------------------
>> 1 file changed, 17 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/security/security_apparmor.c
b/src/security/security_apparmor.c
>> index ad50b08..f871e60 100644
>> --- a/src/security/security_apparmor.c
>> +++ b/src/security/security_apparmor.c
>> @@ -289,10 +289,7 @@ reload_profile(virSecurityManagerPtr mgr,
>> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(
>> def, SECURITY_APPARMOR_NAME);
>>
>> - if (!secdef)
>> - return rc;
>> -
>> - if (!secdef->relabel)
>> + if (!secdef || !secdef->relabel)
>> return 0;
>>
>> if ((profile_name = get_profile_name(def)) == NULL)
>> @@ -435,7 +432,7 @@ AppArmorGenSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
>> SECURITY_APPARMOR_NAME);
>>
>> if (!secdef)
>> - return -1;
>> + return 0;
>>
>> if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) ||
>> (secdef->type == VIR_DOMAIN_SECLABEL_NONE))
>> @@ -495,10 +492,7 @@ AppArmorSetSecurityAllLabel(virSecurityManagerPtr mgr,
>> {
>> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
>> SECURITY_APPARMOR_NAME);
>> - if (!secdef)
>> - return -1;
>> -
>> - if (!secdef->relabel)
>> + if (!secdef || !secdef->relabel)
>> return 0;
>>
>> /* Reload the profile if stdin_path is specified. Note that
>> @@ -559,12 +553,11 @@ AppArmorReleaseSecurityLabel(virSecurityManagerPtr mgr
ATTRIBUTE_UNUSED,
>> {
>> virSecurityLabelDefPtr secdef = virDomainDefGetSecurityLabelDef(def,
>>
SECURITY_APPARMOR_NAME);
>> - if (!secdef)
>> - return -1;
>> -
>> - VIR_FREE(secdef->model);
>> - VIR_FREE(secdef->label);
>> - VIR_FREE(secdef->imagelabel);
>> + if (secdef) {
>> + VIR_FREE(secdef->model);
>> + VIR_FREE(secdef->label);
>> + VIR_FREE(secdef->imagelabel);\
>
> The trailing slash can be dropped here. The rest of th code looks good
> to me.
Opps, thanks for catching that. I've removed it in my local branch.
Do we need someone else with knowledge of the apparmor driver to review and ACK
these changes?
It would be great if Jamie could comment as well in case we're
overlooking any details. Otherwise I think we're good to go.
Cheers,
-- Guido
9:45 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
On 02/03/2017 06:32 PM, Jim Fehlig wrote:
If the apparmor security driver is loaded/enabled and domain config
contains a <seclabel> element whose type attribute is not 'apparmor',
starting the domain fails when attempting to label resources such
as tap FDs.
Many of the apparmor driver entry points attempt to retrieve the
apparmor security label from the domain def, returning failure if
not found. Functions such as AppArmorSetFDLabel fail even though
domain config contains an explicit 'none' secuirty driver, e.g.
<seclabel type='none' model='none'/>
Change the entry points to succeed if the domain config <seclabel>
is not apparmor. This matches the behavior of the selinux driver.
Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
---
src/security/security_apparmor.c | 58 ++++++++++++----------------------------
1 file changed, 17 insertions(+), 41 deletions(-)
ACK
Michal
10:31 a.m.
New subject: [libvirt] [PATCH 2/2] apparmor: don't fail on non-apparmor <seclabel>
Michal Privoznik wrote:
On 02/03/2017 06:32 PM, Jim Fehlig wrote:
> If the apparmor security driver is loaded/enabled and domain config
> contains a <seclabel> element whose type attribute is not 'apparmor',
> starting the domain fails when attempting to label resources such
> as tap FDs.
>
> Many of the apparmor driver entry points attempt to retrieve the
> apparmor security label from the domain def, returning failure if
> not found. Functions such as AppArmorSetFDLabel fail even though
> domain config contains an explicit 'none' secuirty driver, e.g.
>
> <seclabel type='none' model='none'/>
>
> Change the entry points to succeed if the domain config <seclabel>
> is not apparmor. This matches the behavior of the selinux driver.
>
> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com>
> ---
> src/security/security_apparmor.c | 58 ++++++++++++----------------------------
> 1 file changed, 17 insertions(+), 41 deletions(-)
ACK
Thanks. Forgot to mention it, but I pushed these patches yesterday after
receiving your ACK.
Regards,
Jim
2837
days inactive
2849
days old
8 comments
3 participants
participants (3)
-
Guido Günther -
Jim Fehlig -
Michal Privoznik