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

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

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