[PATCH] security: don't fail if built without attr support

Christian Ehrhardt posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200526074730.3490799-1-christian.ehrhardt@canonical.com
There is a newer version of this series
src/security/security_dac.c | 6 ++++++
1 file changed, 6 insertions(+)
[PATCH] security: don't fail if built without attr support
Posted by Christian Ehrhardt 3 years, 10 months ago
If built without attr support removing any image will trigger
 qemuBlockRemoveImageMetadata (the one that emits the warning)
   -> qemuSecurityMoveImageMetadata
     -> virSecurityManagerMoveImageMetadata
       -> virSecurityDACMoveImageMetadata
         -> virSecurityDACMoveImageMetadataHelper
           -> virProcessRunInFork (spawns subprocess)
             -> virSecurityMoveRememberedLabel

In there due to !HAVE_LIBATTR virFileGetXAttrQuiet will return
ENOSYS and from there the chain will error out.

That is wrong and looks like:
  libvirtd[6320]: internal error: child reported (status=125):
  libvirtd[6320]: Unable to remove disk metadata on vm testguest from
  /var/lib/uvtool/libvirt/images/testguest.qcow (disk target vda)

This change makes virSecurityDACMoveImageMetadataHelper accept that
error code gracefully and in that sense it is an extension of:
5214b2f1a3f "security: Don't skip label restore on file systems lacking XATTRs"
which does the same for other call chains into the virFile*XAttr functions.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 src/security/security_dac.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index bdc2d7edf3..7b95a6f86d 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -1117,6 +1117,12 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
 
     ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst);
     virSecurityManagerMetadataUnlock(data->mgr, &state);
+
+    if (ret == -2) {
+        /* Libvirt built without XATTRS */
+        ret = 0;
+    }
+
     return ret;
 }
 
-- 
2.26.0

Re: [PATCH] security: don't fail if built without attr support
Posted by Michal Privoznik 3 years, 10 months ago
On 5/26/20 9:47 AM, Christian Ehrhardt wrote:
> If built without attr support removing any image will trigger
>   qemuBlockRemoveImageMetadata (the one that emits the warning)
>     -> qemuSecurityMoveImageMetadata
>       -> virSecurityManagerMoveImageMetadata
>         -> virSecurityDACMoveImageMetadata
>           -> virSecurityDACMoveImageMetadataHelper
>             -> virProcessRunInFork (spawns subprocess)
>               -> virSecurityMoveRememberedLabel
> 
> In there due to !HAVE_LIBATTR virFileGetXAttrQuiet will return
> ENOSYS and from there the chain will error out.
> 
> That is wrong and looks like:
>    libvirtd[6320]: internal error: child reported (status=125):
>    libvirtd[6320]: Unable to remove disk metadata on vm testguest from
>    /var/lib/uvtool/libvirt/images/testguest.qcow (disk target vda)
> 
> This change makes virSecurityDACMoveImageMetadataHelper accept that
> error code gracefully and in that sense it is an extension of:
> 5214b2f1a3f "security: Don't skip label restore on file systems lacking XATTRs"
> which does the same for other call chains into the virFile*XAttr functions.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>   src/security/security_dac.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index bdc2d7edf3..7b95a6f86d 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -1117,6 +1117,12 @@ virSecurityDACMoveImageMetadataHelper(pid_t pid G_GNUC_UNUSED,
>   
>       ret = virSecurityMoveRememberedLabel(SECURITY_DAC_NAME, data->src, data->dst);
>       virSecurityManagerMetadataUnlock(data->mgr, &state);
> +
> +    if (ret == -2) {
> +        /* Libvirt built without XATTRS */
> +        ret = 0;
> +    }
> +
>       return ret;
>   }
>   
> 

The SELinux driver can use the same treatment (although you won't hit it 
because ubuntu uses apparmor instead). You have my Reviewed by if you do 
the same change to virSecuritySELinuxMoveImageMetadataHelper().

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal