[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata

Michal Privoznik 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/51de9763454e44a1e99eba2337a6f2baaa6a30ad.1589557454.git.mprivozn@redhat.com
src/security/security_manager.c |  3 +--
src/security/security_nop.c     | 10 ----------
2 files changed, 1 insertion(+), 12 deletions(-)
[PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Michal Privoznik 3 years, 10 months ago
The AppArmor secdriver does not use labels to grant access to
resources. Therefore, it doesn't use XATTRs and hence it lacks
implementation of .domainMoveImageMetadata callback. This leads
to a harmless but needless error message appearing in the logs:

  virSecurityManagerMoveImageMetadata:476 : this function is not
  supported by the connection driver: virSecurityManagerMoveImageMetadata

Closes: https://gitlab.com/libvirt/libvirt/-/issues/25

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/security/security_manager.c |  3 +--
 src/security/security_nop.c     | 10 ----------
 2 files changed, 1 insertion(+), 12 deletions(-)

diff --git a/src/security/security_manager.c b/src/security/security_manager.c
index 2dea294784..b1237d63b6 100644
--- a/src/security/security_manager.c
+++ b/src/security/security_manager.c
@@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
         return ret;
     }
 
-    virReportUnsupportedError();
-    return -1;
+    return 0;
 }
 
 
diff --git a/src/security/security_nop.c b/src/security/security_nop.c
index c1856eb421..d5f715b916 100644
--- a/src/security/security_nop.c
+++ b/src/security/security_nop.c
@@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
     return 0;
 }
 
-static int
-virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
-                                      pid_t pid G_GNUC_UNUSED,
-                                      virStorageSourcePtr src G_GNUC_UNUSED,
-                                      virStorageSourcePtr dst G_GNUC_UNUSED)
-{
-    return 0;
-}
-
 static int
 virSecurityDomainSetMemoryLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
                                    virDomainDefPtr def G_GNUC_UNUSED,
@@ -290,7 +281,6 @@ virSecurityDriver virSecurityDriverNop = {
 
     .domainSetSecurityImageLabel        = virSecurityDomainSetImageLabelNop,
     .domainRestoreSecurityImageLabel    = virSecurityDomainRestoreImageLabelNop,
-    .domainMoveImageMetadata            = virSecurityDomainMoveImageMetadataNop,
 
     .domainSetSecurityMemoryLabel       = virSecurityDomainSetMemoryLabelNop,
     .domainRestoreSecurityMemoryLabel   = virSecurityDomainRestoreMemoryLabelNop,
-- 
2.26.2

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Erik Skultety 3 years, 10 months ago
On Fri, May 15, 2020 at 05:44:38PM +0200, Michal Privoznik wrote:
> The AppArmor secdriver does not use labels to grant access to
> resources. Therefore, it doesn't use XATTRs and hence it lacks
> implementation of .domainMoveImageMetadata callback. This leads
> to a harmless but needless error message appearing in the logs:
> 
>   virSecurityManagerMoveImageMetadata:476 : this function is not
>   supported by the connection driver: virSecurityManagerMoveImageMetadata
> 
> Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/security/security_manager.c |  3 +--
>  src/security/security_nop.c     | 10 ----------
>  2 files changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 2dea294784..b1237d63b6 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
>          return ret;
>      }
>  
> -    virReportUnsupportedError();
> -    return -1;
> +    return 0;
>  }

To ^this hunk:
Reviewed-by: Erik Skultety <eskultet@redhat.com>

>  
>  
> diff --git a/src/security/security_nop.c b/src/security/security_nop.c
> index c1856eb421..d5f715b916 100644
> --- a/src/security/security_nop.c
> +++ b/src/security/security_nop.c
> @@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>      return 0;
>  }
>  
> -static int
> -virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> -                                      pid_t pid G_GNUC_UNUSED,
> -                                      virStorageSourcePtr src G_GNUC_UNUSED,
> -                                      virStorageSourcePtr dst G_GNUC_UNUSED)
> -{
> -    return 0;
> -}
> -

^This is an unrelated change and I also think that the Nop driver should
implement (ideally) all the functions, so I don't see a reason in removing
this one, otherwise we should remove more than just this very function.

-- 
Erik Skultety

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Michal Privoznik 3 years, 10 months ago
On 5/18/20 8:16 AM, Erik Skultety wrote:
> On Fri, May 15, 2020 at 05:44:38PM +0200, Michal Privoznik wrote:
>> The AppArmor secdriver does not use labels to grant access to
>> resources. Therefore, it doesn't use XATTRs and hence it lacks
>> implementation of .domainMoveImageMetadata callback. This leads
>> to a harmless but needless error message appearing in the logs:
>>
>>    virSecurityManagerMoveImageMetadata:476 : this function is not
>>    supported by the connection driver: virSecurityManagerMoveImageMetadata
>>
>> Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/security/security_manager.c |  3 +--
>>   src/security/security_nop.c     | 10 ----------
>>   2 files changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
>> index 2dea294784..b1237d63b6 100644
>> --- a/src/security/security_manager.c
>> +++ b/src/security/security_manager.c
>> @@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
>>           return ret;
>>       }
>>   
>> -    virReportUnsupportedError();
>> -    return -1;
>> +    return 0;
>>   }
> 
> To ^this hunk:
> Reviewed-by: Erik Skultety <eskultet@redhat.com>
> 
>>   
>>   
>> diff --git a/src/security/security_nop.c b/src/security/security_nop.c
>> index c1856eb421..d5f715b916 100644
>> --- a/src/security/security_nop.c
>> +++ b/src/security/security_nop.c
>> @@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>>       return 0;
>>   }
>>   
>> -static int
>> -virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
>> -                                      pid_t pid G_GNUC_UNUSED,
>> -                                      virStorageSourcePtr src G_GNUC_UNUSED,
>> -                                      virStorageSourcePtr dst G_GNUC_UNUSED)
>> -{
>> -    return 0;
>> -}
>> -
> 
> ^This is an unrelated change and I also think that the Nop driver should
> implement (ideally) all the functions, so I don't see a reason in removing
> this one, otherwise we should remove more than just this very function.
> 

It's not unrelated. The NOP driver is always present (see 
security_drivers[] in src/security/security_driver.c). Therefore, this 
dummy implementation was needed because if no other driver was loaded 
then NOP implementation saved us from reporting unsupported error.

But you are right that the important bit is the first hunk. So whatever 
you prefer.

Michal

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Erik Skultety 3 years, 10 months ago
On Mon, May 18, 2020 at 09:26:20AM +0200, Michal Privoznik wrote:
> On 5/18/20 8:16 AM, Erik Skultety wrote:
> > On Fri, May 15, 2020 at 05:44:38PM +0200, Michal Privoznik wrote:
> > > The AppArmor secdriver does not use labels to grant access to
> > > resources. Therefore, it doesn't use XATTRs and hence it lacks
> > > implementation of .domainMoveImageMetadata callback. This leads
> > > to a harmless but needless error message appearing in the logs:
> > > 
> > >    virSecurityManagerMoveImageMetadata:476 : this function is not
> > >    supported by the connection driver: virSecurityManagerMoveImageMetadata
> > > 
> > > Closes: https://gitlab.com/libvirt/libvirt/-/issues/25
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   src/security/security_manager.c |  3 +--
> > >   src/security/security_nop.c     | 10 ----------
> > >   2 files changed, 1 insertion(+), 12 deletions(-)
> > > 
> > > diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> > > index 2dea294784..b1237d63b6 100644
> > > --- a/src/security/security_manager.c
> > > +++ b/src/security/security_manager.c
> > > @@ -473,8 +473,7 @@ virSecurityManagerMoveImageMetadata(virSecurityManagerPtr mgr,
> > >           return ret;
> > >       }
> > > -    virReportUnsupportedError();
> > > -    return -1;
> > > +    return 0;
> > >   }
> > 
> > To ^this hunk:
> > Reviewed-by: Erik Skultety <eskultet@redhat.com>
> > 
> > > diff --git a/src/security/security_nop.c b/src/security/security_nop.c
> > > index c1856eb421..d5f715b916 100644
> > > --- a/src/security/security_nop.c
> > > +++ b/src/security/security_nop.c
> > > @@ -225,15 +225,6 @@ virSecurityDomainSetImageLabelNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > >       return 0;
> > >   }
> > > -static int
> > > -virSecurityDomainMoveImageMetadataNop(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> > > -                                      pid_t pid G_GNUC_UNUSED,
> > > -                                      virStorageSourcePtr src G_GNUC_UNUSED,
> > > -                                      virStorageSourcePtr dst G_GNUC_UNUSED)
> > > -{
> > > -    return 0;
> > > -}
> > > -
> > 
> > ^This is an unrelated change and I also think that the Nop driver should
> > implement (ideally) all the functions, so I don't see a reason in removing
> > this one, otherwise we should remove more than just this very function.
> > 
> 
> It's not unrelated. The NOP driver is always present (see security_drivers[]
> in src/security/security_driver.c). Therefore, this dummy implementation was
> needed because if no other driver was loaded then NOP implementation saved
> us from reporting unsupported error.

Yes, I know, what I meant by "unrelated" here was just the fact that in order
to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
next time :). It's true that with the first hunk the second becomes redundant,
but I still feel like the NOP driver should cover the full spectrum of
operations we support, but maybe I'm trying to be overly cautious here.

-- 
Erik Skultety

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Michal Privoznik 3 years, 10 months ago
On 5/18/20 10:02 AM, Erik Skultety wrote:

> Yes, I know, what I meant by "unrelated" here was just the fact that in order
> to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
> next time :). It's true that with the first hunk the second becomes redundant,
> but I still feel like the NOP driver should cover the full spectrum of
> operations we support, but maybe I'm trying to be overly cautious here.
> 

Well, it doesn't implement everything already. But okay, I have ACK for 
the important hunk so I will push only that one.

Thanks!
Michal

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Christian Ehrhardt 3 years, 10 months ago
On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/18/20 10:02 AM, Erik Skultety wrote:
>
> > Yes, I know, what I meant by "unrelated" here was just the fact that in order
> > to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
> > next time :). It's true that with the first hunk the second becomes redundant,
> > but I still feel like the NOP driver should cover the full spectrum of
> > operations we support, but maybe I'm trying to be overly cautious here.
> >
>
> Well, it doesn't implement everything already. But okay, I have ACK for
> the important hunk so I will push only that one.

Hi Michal,
while debugging a Ubuntu bug report I found that this fix will
mitigate the warning you wanted to fix.
But overall there still is an issue with the labeling introduced by
[1][2] in >=5.6.

The situation (related to this fix, hence replying in this context) is
the following.
Ubuntu (as an example) builds and has build with --without-attr.

That will make the code have !HAVE_LIBATTR which was fine in the past.
But with your code added the LP bug [3] identified an issue.

What happens is that in stacked security it will iterate on:
- virSecurityStackMoveImageMetadata (for the stacking itself)
- 0x0 (apparmor)
- virSecurityDACMoveImageMetadata

The fix discussed here fixes the warning emitted by the apparmor case like:
"this function is not supported by the connection driver"

But when iterating further on a build that has no attr support we
encounter the following (e.g. at guest shutdown):

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)

I found that this is due to:
qemuBlockRemoveImageMetadata (the one that emits the warning)
  -> qemuSecurityMoveImageMetadata
    -> virSecurityManagerMoveImageMetadata
      -> virSecurityDACMoveImageMetadata
        -> virSecurityDACMoveImageMetadataHelper
          -> virProcessRunInFork (spawns subprocess)
            -> virSecurityMoveRememberedLabel

Since this is built without HAVE_LIBATTR the following will happen

461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
(gdb) n
462 if (errno == ENOSYS || errno == ENOTSUP) {
(gdb) p errno
$32 = 38

And that is due to !HAVE_LIBATTR which maps the implementation onto:

4412 #else /* !HAVE_LIBATTR */
4413
4414 int
4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
4416 const char *name G_GNUC_UNUSED,
4417 char **value G_GNUC_UNUSED)
4418 {
4419 errno = ENOSYS;
4420 return -1;
4421 }

Due to that we see the two messages reported above
a) internal errors -> for the subprocess that failed
b) "Unable to remove disk metadata" -> but this time due to DAC
instead of apparmor in the security stack

I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
Con: That would still fork the process to do nothing then
Pro: It would but be a small change in just one place

Since you did all the related changes I thought I report the case and
leave it up to you Michal, what do you think?


[1]: https://gitlab.com/libvirt/libvirt/-/commit/706e68237f5
[2]: https://gitlab.com/libvirt/libvirt/-/commit/d73f3f58360
[3]: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/1879325

> Thanks!
> Michal
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Michal Privoznik 3 years, 10 months ago
On 5/25/20 1:14 PM, Christian Ehrhardt wrote:
> On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/18/20 10:02 AM, Erik Skultety wrote:
>>
>>> Yes, I know, what I meant by "unrelated" here was just the fact that in order
>>> to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
>>> next time :). It's true that with the first hunk the second becomes redundant,
>>> but I still feel like the NOP driver should cover the full spectrum of
>>> operations we support, but maybe I'm trying to be overly cautious here.
>>>
>>
>> Well, it doesn't implement everything already. But okay, I have ACK for
>> the important hunk so I will push only that one.
> 
> Hi Michal,
> while debugging a Ubuntu bug report I found that this fix will
> mitigate the warning you wanted to fix.
> But overall there still is an issue with the labeling introduced by
> [1][2] in >=5.6.
> 
> The situation (related to this fix, hence replying in this context) is
> the following.
> Ubuntu (as an example) builds and has build with --without-attr.
> 
> That will make the code have !HAVE_LIBATTR which was fine in the past.
> But with your code added the LP bug [3] identified an issue.
> 
> What happens is that in stacked security it will iterate on:
> - virSecurityStackMoveImageMetadata (for the stacking itself)
> - 0x0 (apparmor)
> - virSecurityDACMoveImageMetadata
> 
> The fix discussed here fixes the warning emitted by the apparmor case like:
> "this function is not supported by the connection driver"
> 
> But when iterating further on a build that has no attr support we
> encounter the following (e.g. at guest shutdown):
> 
> 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)
> 
> I found that this is due to:
> qemuBlockRemoveImageMetadata (the one that emits the warning)
>    -> qemuSecurityMoveImageMetadata
>      -> virSecurityManagerMoveImageMetadata
>        -> virSecurityDACMoveImageMetadata
>          -> virSecurityDACMoveImageMetadataHelper
>            -> virProcessRunInFork (spawns subprocess)
>              -> virSecurityMoveRememberedLabel
> 
> Since this is built without HAVE_LIBATTR the following will happen
> 
> 461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
> (gdb) n
> 462 if (errno == ENOSYS || errno == ENOTSUP) {
> (gdb) p errno
> $32 = 38
> 
> And that is due to !HAVE_LIBATTR which maps the implementation onto:
> 
> 4412 #else /* !HAVE_LIBATTR */
> 4413
> 4414 int
> 4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
> 4416 const char *name G_GNUC_UNUSED,
> 4417 char **value G_GNUC_UNUSED)
> 4418 {
> 4419 errno = ENOSYS;
> 4420 return -1;
> 4421 }
> 
> Due to that we see the two messages reported above
> a) internal errors -> for the subprocess that failed
> b) "Unable to remove disk metadata" -> but this time due to DAC
> instead of apparmor in the security stack

Thank you for your deep analysis.

> 
> I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
> virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
> Con: That would still fork the process to do nothing then
> Pro: It would but be a small change in just one place
> 
> Since you did all the related changes I thought I report the case and
> leave it up to you Michal, what do you think?

Yes, a naive fix would be something like this:

diff --git i/src/security/security_dac.c w/src/security/security_dac.c
index bdc2d7edf3..7b95a6f86d 100644
--- i/src/security/security_dac.c
+++ w/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;
  }


But as you correctly say, it will still lead to an unnecessary spawn of 
a process that will do NOP (basically). I think a better fix would be to 
not require .domainMoveImageMetadata callbacks (i.e. the one from NOP 
driver could be then removed) and set the DAC/SELinux callbacks if and 
only if HAVE_LIBATTR; alternatively, make those functions be NOP if 
!HAVE_LIBATTR.

On the other hand, every time we relabel a path outside of /dev, we 
technically spawn unnecessary because only /dev lives inside a private 
NS. Everything else is from the parent NS. For instance, there is no 
need to spawn only to chown("/var/lib/libvirt/images/fedora.qcow2").

Therefore I might have a slight preference for the naive fix, but I will 
leave it up to you. Or do you want me to sent the patch?

Michal

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Christian Ehrhardt 3 years, 10 months ago
On Mon, May 25, 2020 at 3:25 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>
> On 5/25/20 1:14 PM, Christian Ehrhardt wrote:
> > On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn@redhat.com> wrote:
> >>
> >> On 5/18/20 10:02 AM, Erik Skultety wrote:
> >>
> >>> Yes, I know, what I meant by "unrelated" here was just the fact that in order
> >>> to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
> >>> next time :). It's true that with the first hunk the second becomes redundant,
> >>> but I still feel like the NOP driver should cover the full spectrum of
> >>> operations we support, but maybe I'm trying to be overly cautious here.
> >>>
> >>
> >> Well, it doesn't implement everything already. But okay, I have ACK for
> >> the important hunk so I will push only that one.
> >
> > Hi Michal,
> > while debugging a Ubuntu bug report I found that this fix will
> > mitigate the warning you wanted to fix.
> > But overall there still is an issue with the labeling introduced by
> > [1][2] in >=5.6.
> >
> > The situation (related to this fix, hence replying in this context) is
> > the following.
> > Ubuntu (as an example) builds and has build with --without-attr.
> >
> > That will make the code have !HAVE_LIBATTR which was fine in the past.
> > But with your code added the LP bug [3] identified an issue.
> >
> > What happens is that in stacked security it will iterate on:
> > - virSecurityStackMoveImageMetadata (for the stacking itself)
> > - 0x0 (apparmor)
> > - virSecurityDACMoveImageMetadata
> >
> > The fix discussed here fixes the warning emitted by the apparmor case like:
> > "this function is not supported by the connection driver"
> >
> > But when iterating further on a build that has no attr support we
> > encounter the following (e.g. at guest shutdown):
> >
> > 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)
> >
> > I found that this is due to:
> > qemuBlockRemoveImageMetadata (the one that emits the warning)
> >    -> qemuSecurityMoveImageMetadata
> >      -> virSecurityManagerMoveImageMetadata
> >        -> virSecurityDACMoveImageMetadata
> >          -> virSecurityDACMoveImageMetadataHelper
> >            -> virProcessRunInFork (spawns subprocess)
> >              -> virSecurityMoveRememberedLabel
> >
> > Since this is built without HAVE_LIBATTR the following will happen
> >
> > 461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
> > (gdb) n
> > 462 if (errno == ENOSYS || errno == ENOTSUP) {
> > (gdb) p errno
> > $32 = 38
> >
> > And that is due to !HAVE_LIBATTR which maps the implementation onto:
> >
> > 4412 #else /* !HAVE_LIBATTR */
> > 4413
> > 4414 int
> > 4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
> > 4416 const char *name G_GNUC_UNUSED,
> > 4417 char **value G_GNUC_UNUSED)
> > 4418 {
> > 4419 errno = ENOSYS;
> > 4420 return -1;
> > 4421 }
> >
> > Due to that we see the two messages reported above
> > a) internal errors -> for the subprocess that failed
> > b) "Unable to remove disk metadata" -> but this time due to DAC
> > instead of apparmor in the security stack
>
> Thank you for your deep analysis.
>
> >
> > I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
> > virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
> > Con: That would still fork the process to do nothing then
> > Pro: It would but be a small change in just one place
> >
> > Since you did all the related changes I thought I report the case and
> > leave it up to you Michal, what do you think?
>
> Yes, a naive fix would be something like this:
>
> diff --git i/src/security/security_dac.c w/src/security/security_dac.c
> index bdc2d7edf3..7b95a6f86d 100644
> --- i/src/security/security_dac.c
> +++ w/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;
>   }
>
>
> But as you correctly say, it will still lead to an unnecessary spawn of
> a process that will do NOP (basically). I think a better fix would be to
> not require .domainMoveImageMetadata callbacks (i.e. the one from NOP
> driver could be then removed) and set the DAC/SELinux callbacks if and
> only if HAVE_LIBATTR; alternatively, make those functions be NOP if
> !HAVE_LIBATTR.
>
> On the other hand, every time we relabel a path outside of /dev, we
> technically spawn unnecessary because only /dev lives inside a private
> NS. Everything else is from the parent NS. For instance, there is no
> need to spawn only to chown("/var/lib/libvirt/images/fedora.qcow2").
>
> Therefore I might have a slight preference for the naive fix, but I will
> leave it up to you. Or do you want me to sent the patch?

Hi,
as mentioned on IRC yesterday I was giving this a look.
While looking deeper at the naive fix I was derailed and probably have
given it too much second though for the naive implementation.

TL;DR:
- we should be ok with the naive fix as suggested

Details:
The low level implementations all have an ifdef on HAVE_LIBATTR to
return -1 and set ENOSYS.
- virFileSetXAttr
- virFileGetXAttrQuiet (the only one not pushing to virReportSystemError)
- virFileRemoveXAttr

Checking their callers I saw a mixed situation.

"rc not checked" => means with out libattr support these will behave
as if it failed.
"rc checked and delivering -2" => means it compares to ENOSYS/ENOTSUP
and returns -2

Converting ENOTSUP to rc=-1:
- virSecurityAddTimestamp
  -> virFileSetXAttr (rc not checked)
- virSecurityRemoveTimestamp
  -> virFileRemoveXAttr (rc not checked)

Early check via virFileGetXAttrQuiet returning -2:
- virSecurityValidateTimestamp
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
- virSecuritySetRememberedLabel
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> 2 x virFileSetXAttr (rc not checked)
- virSecurityGetRememberedLabel
  -> virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> virFileSetXAttr (rc not checked)
  -> virFileRemoveXAttr (rc not checked)
- virSecurityMoveRememberedLabel
  -> 3 x virFileGetXAttrQuiet ("rc checked and delivering -2")
  -> 3 x virFileSetXAttr (rc not checked)
  -> 3 x virFileRemoveXAttr (rc not checked)
  -> 3 x virFileRemoveXAttr (rc ignored without consequence)

It might have been meant to use virFileGetXAttrQuiet (no error report)
as probing function to early exit.
It is usually used early and would thereby return -2.

But can we be sure that e.g. virSecurityAddTimestamp and
virSecurityRemoveTimestamp are never called without HAVE_LIBATTR.
Since those would just ignore the ENOTSUP/ENOSYS and make it a bad
rc=1. They might trigger a similar issue to the one I reported. Do
they need the same "if ENOTSUP/ENOSYS then RC=-2"?
I checked the callers and as of today it seems to be only
virSecurityGetRememberedLabel and virSecuritySetRememberedLabel wich
would before probe via virFileGetXAttrQuiet.
So might be safe on these.

But furthermore even when -2 is passed back, as my case has shown the
RC of virSecurityMoveRememberedLabel wasn't checked for RC==-2 to be
handled gracefully in virSecurityDACMoveImageMetadataHelper.
I checked all the callers of the above list that returns -2, they all
ignore the special case.
So instead of just virSecurityDACMoveImageMetadataHelper as discussed
we might need the same/similar change in:

- virSecuritySELinuxRememberLabel
- virSecuritySELinuxRecallLabel
- virSecuritySELinuxMoveImageMetadataHelper
(SELinux without XATTR isn't really a thing I guess - so we might skip
these, but I'm not sure)

- virSecurityDACRememberLabel -> rc=-2 covered in virSecurityDACSetOwnership
- virSecurityDACRecallLabel -> rc=-2 covered in
virSecurityDACRestoreFileLabelInternal

I was first afraid if "just fixing"
virSecurityDACMoveImageMetadataHelper might fix the case I've found
while there might be more cases just waiting to trigger with different
use cases. But it seems we are good in that regard.
But after a long trip through the code it seems just
virSecurityDACMoveImageMetadataHelper should be ok.
It comes down to officially submitting the change Michal suggested ...

@Michal - if anything above rings a bell and "should work differently"
speak up here or on the patch that follows and feel free to provide a
better fix then adapting to whatever is different than I assumed.


> Michal
>


--
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd

Re: [PATCH] Don't require secdrivers to implement .domainMoveImageMetadata
Posted by Michal Privoznik 3 years, 10 months ago
On 5/26/20 9:46 AM, Christian Ehrhardt wrote:
> On Mon, May 25, 2020 at 3:25 PM Michal Privoznik <mprivozn@redhat.com> wrote:
>>
>> On 5/25/20 1:14 PM, Christian Ehrhardt wrote:
>>> On Mon, May 18, 2020 at 10:07 AM Michal Privoznik <mprivozn@redhat.com> wrote:
>>>>
>>>> On 5/18/20 10:02 AM, Erik Skultety wrote:
>>>>
>>>>> Yes, I know, what I meant by "unrelated" here was just the fact that in order
>>>>> to fix Apparmor, you only need the first hunk, I guess I'll be more explicit
>>>>> next time :). It's true that with the first hunk the second becomes redundant,
>>>>> but I still feel like the NOP driver should cover the full spectrum of
>>>>> operations we support, but maybe I'm trying to be overly cautious here.
>>>>>
>>>>
>>>> Well, it doesn't implement everything already. But okay, I have ACK for
>>>> the important hunk so I will push only that one.
>>>
>>> Hi Michal,
>>> while debugging a Ubuntu bug report I found that this fix will
>>> mitigate the warning you wanted to fix.
>>> But overall there still is an issue with the labeling introduced by
>>> [1][2] in >=5.6.
>>>
>>> The situation (related to this fix, hence replying in this context) is
>>> the following.
>>> Ubuntu (as an example) builds and has build with --without-attr.
>>>
>>> That will make the code have !HAVE_LIBATTR which was fine in the past.
>>> But with your code added the LP bug [3] identified an issue.
>>>
>>> What happens is that in stacked security it will iterate on:
>>> - virSecurityStackMoveImageMetadata (for the stacking itself)
>>> - 0x0 (apparmor)
>>> - virSecurityDACMoveImageMetadata
>>>
>>> The fix discussed here fixes the warning emitted by the apparmor case like:
>>> "this function is not supported by the connection driver"
>>>
>>> But when iterating further on a build that has no attr support we
>>> encounter the following (e.g. at guest shutdown):
>>>
>>> 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)
>>>
>>> I found that this is due to:
>>> qemuBlockRemoveImageMetadata (the one that emits the warning)
>>>     -> qemuSecurityMoveImageMetadata
>>>       -> virSecurityManagerMoveImageMetadata
>>>         -> virSecurityDACMoveImageMetadata
>>>           -> virSecurityDACMoveImageMetadataHelper
>>>             -> virProcessRunInFork (spawns subprocess)
>>>               -> virSecurityMoveRememberedLabel
>>>
>>> Since this is built without HAVE_LIBATTR the following will happen
>>>
>>> 461 if (virFileGetXAttrQuiet(src, ref_name, &ref_value) < 0) {
>>> (gdb) n
>>> 462 if (errno == ENOSYS || errno == ENOTSUP) {
>>> (gdb) p errno
>>> $32 = 38
>>>
>>> And that is due to !HAVE_LIBATTR which maps the implementation onto:
>>>
>>> 4412 #else /* !HAVE_LIBATTR */
>>> 4413
>>> 4414 int
>>> 4415 virFileGetXAttrQuiet(const char *path G_GNUC_UNUSED,
>>> 4416 const char *name G_GNUC_UNUSED,
>>> 4417 char **value G_GNUC_UNUSED)
>>> 4418 {
>>> 4419 errno = ENOSYS;
>>> 4420 return -1;
>>> 4421 }
>>>
>>> Due to that we see the two messages reported above
>>> a) internal errors -> for the subprocess that failed
>>> b) "Unable to remove disk metadata" -> but this time due to DAC
>>> instead of apparmor in the security stack
>>
>> Thank you for your deep analysis.
>>
>>>
>>> I'm not sure what you'd prefer Michal, maybe an early RC=0 exit in
>>> virSecurityMoveRememberedLabel in case of !HAVE_LIBATTR?
>>> Con: That would still fork the process to do nothing then
>>> Pro: It would but be a small change in just one place
>>>
>>> Since you did all the related changes I thought I report the case and
>>> leave it up to you Michal, what do you think?
>>
>> Yes, a naive fix would be something like this:
>>
>> diff --git i/src/security/security_dac.c w/src/security/security_dac.c
>> index bdc2d7edf3..7b95a6f86d 100644
>> --- i/src/security/security_dac.c
>> +++ w/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;
>>    }
>>
>>
>> But as you correctly say, it will still lead to an unnecessary spawn of
>> a process that will do NOP (basically). I think a better fix would be to
>> not require .domainMoveImageMetadata callbacks (i.e. the one from NOP
>> driver could be then removed) and set the DAC/SELinux callbacks if and
>> only if HAVE_LIBATTR; alternatively, make those functions be NOP if
>> !HAVE_LIBATTR.
>>
>> On the other hand, every time we relabel a path outside of /dev, we
>> technically spawn unnecessary because only /dev lives inside a private
>> NS. Everything else is from the parent NS. For instance, there is no
>> need to spawn only to chown("/var/lib/libvirt/images/fedora.qcow2").
>>
>> Therefore I might have a slight preference for the naive fix, but I will
>> leave it up to you. Or do you want me to sent the patch?
> 
> Hi,
> as mentioned on IRC yesterday I was giving this a look.
> While looking deeper at the naive fix I was derailed and probably have
> given it too much second though for the naive implementation.
> 
> TL;DR:
> - we should be ok with the naive fix as suggested
> 
> Details:
> The low level implementations all have an ifdef on HAVE_LIBATTR to
> return -1 and set ENOSYS.
> - virFileSetXAttr
> - virFileGetXAttrQuiet (the only one not pushing to virReportSystemError)
> - virFileRemoveXAttr
> 
> Checking their callers I saw a mixed situation.
> 
> "rc not checked" => means with out libattr support these will behave
> as if it failed.
> "rc checked and delivering -2" => means it compares to ENOSYS/ENOTSUP
> and returns -2
> 
> Converting ENOTSUP to rc=-1:
> - virSecurityAddTimestamp
>    -> virFileSetXAttr (rc not checked)
> - virSecurityRemoveTimestamp
>    -> virFileRemoveXAttr (rc not checked)
> 
> Early check via virFileGetXAttrQuiet returning -2:
> - virSecurityValidateTimestamp
>    -> virFileGetXAttrQuiet ("rc checked and delivering -2")
> - virSecuritySetRememberedLabel
>    -> virFileGetXAttrQuiet ("rc checked and delivering -2")
>    -> 2 x virFileSetXAttr (rc not checked)
> - virSecurityGetRememberedLabel
>    -> virFileGetXAttrQuiet ("rc checked and delivering -2")
>    -> virFileSetXAttr (rc not checked)
>    -> virFileRemoveXAttr (rc not checked)
> - virSecurityMoveRememberedLabel
>    -> 3 x virFileGetXAttrQuiet ("rc checked and delivering -2")
>    -> 3 x virFileSetXAttr (rc not checked)
>    -> 3 x virFileRemoveXAttr (rc not checked)
>    -> 3 x virFileRemoveXAttr (rc ignored without consequence)
> 
> It might have been meant to use virFileGetXAttrQuiet (no error report)
> as probing function to early exit.
> It is usually used early and would thereby return -2.
> 
> But can we be sure that e.g. virSecurityAddTimestamp and
> virSecurityRemoveTimestamp are never called without HAVE_LIBATTR.
> Since those would just ignore the ENOTSUP/ENOSYS and make it a bad
> rc=1. They might trigger a similar issue to the one I reported. Do
> they need the same "if ENOTSUP/ENOSYS then RC=-2"?
> I checked the callers and as of today it seems to be only
> virSecurityGetRememberedLabel and virSecuritySetRememberedLabel wich
> would before probe via virFileGetXAttrQuiet.
> So might be safe on these.

Yeah, the idea is that at the beginning of every Rememember/Recall/Move 
Label call, there is virFileGetXAttrQuiet() which intentionally doesn't 
report error, not only because HAVE_LIBATTR might be unset, but also 
because the underlying FS might not support XATTRs. After that it is 
safe (and in fact desired) for every Set/Get XATTR call to return an error.

> 
> But furthermore even when -2 is passed back, as my case has shown the
> RC of virSecurityMoveRememberedLabel wasn't checked for RC==-2 to be
> handled gracefully in virSecurityDACMoveImageMetadataHelper.
> I checked all the callers of the above list that returns -2, they all
> ignore the special case.
> So instead of just virSecurityDACMoveImageMetadataHelper as discussed
> we might need the same/similar change in:
> 
> - virSecuritySELinuxRememberLabel
> - virSecuritySELinuxRecallLabel

Aren't these handled too, similarly to how DAC driver handles them?

> - virSecuritySELinuxMoveImageMetadataHelper

This one needs fixing, as I mention in my review of your patch.

> (SELinux without XATTR isn't really a thing I guess - so we might skip
> these, but I'm not sure)
> 
> - virSecurityDACRememberLabel -> rc=-2 covered in virSecurityDACSetOwnership
> - virSecurityDACRecallLabel -> rc=-2 covered in
> virSecurityDACRestoreFileLabelInternal
> 
> I was first afraid if "just fixing"
> virSecurityDACMoveImageMetadataHelper might fix the case I've found
> while there might be more cases just waiting to trigger with different
> use cases. But it seems we are good in that regard.
> But after a long trip through the code it seems just
> virSecurityDACMoveImageMetadataHelper should be ok.
> It comes down to officially submitting the change Michal suggested ...
> 
> @Michal - if anything above rings a bell and "should work differently"
> speak up here or on the patch that follows and feel free to provide a
> better fix then adapting to whatever is different than I assumed.

I think the patch is good.

Michal