On Mon, Jul 01, 2013 at 02:28:46PM +0200, Peter Krempa wrote:
> On 06/27/13 20:51, Daniel P. Berrange wrote:
> >On Wed, Jun 26, 2013 at 03:01:49PM +0200, Peter Krempa wrote:
> >>The method labels the file descriptor even if dynamic labeling/relabeling
> >>is turned off. This is needed for files created by libvirt and then
> >>passed along to qemu as a FD.
> >>---
> >> src/libvirt_private.syms | 1 +
> >> src/security/security_dac.c | 9 +++++++++
> >> src/security/security_driver.h | 4 ++++
> >> src/security/security_manager.c | 16 ++++++++++++++++
> >> src/security/security_manager.h | 3 +++
> >> src/security/security_nop.c | 1 +
> >> src/security/security_selinux.c | 21 +++++++++++++++++++++
> >> src/security/security_stack.c | 19 +++++++++++++++++++
> >> 8 files changed, 74 insertions(+)
> >>
> >
> >>diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
> >>index 7802dda..5894259 100644
> >>--- a/src/security/security_selinux.c
> >>+++ b/src/security/security_selinux.c
> >>@@ -2446,6 +2446,26 @@ virSecuritySELinuxGetSecurityMountOptions(virSecurityManagerPtr mgr,
> >> return opts;
> >> }
> >>
> >>+static int
> >>+virSecuritySELinuxSetCreatedFDLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED,
> >>+ virDomainDefPtr def,
> >>+ int fd)
> >>+{
> >>+ virSecurityLabelDefPtr secdef;
> >>+
> >>+ if ((secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME))) {
> >>+ if (!secdef->imagelabel)
> >>+ secdef->imagelabel = virSecuritySELinuxGenImageLabel(mgr, def);
> >
> >This is really dubious. None of the methods except for GenSecurityLabel
> >should be making changes to the secdef state.
>
> There is already an exception: virSecuritySELinuxGetSecurityMountOptions().
That is a not an example of good code & needs to be fixed for the
same reason.
> >
> >>+
> >>+ if (secdef->imagelabel == NULL)
> >>+ return 0;
> >>+
> >>+ return virSecuritySELinuxFSetFilecon(fd, secdef->imagelabel);
> >>+}
> >
> >
> >In general I'm not really convinced we should be adding a new method
> >here, as opposed to making the existing SetImageFDLabel do the right
> >thing.
>
> I was going to suggest expanding the function prototype with a bool
> that would allow to choose whether to label the image always or only
> if relabeling is enabled but looking through the code I noticed that
> actually every caller would need to have this flag enabled.
>
> There are currently 4 places this method is called:
>
> qemuDumpToFd() - labels a FD of a freshly created file for the memory dump
>
> qemuMigrationToFile() - again a new file FD is labeled, used for
> (managed) save and dumping of memory
>
> doTunnelMigrate() - the FD's of a pipe used for extracting data into
> the stream are labeled, again this would probably fail with static
> labeling.
>
> qemuProcessStart() - the migration FD is labelled on process start.
>
> I think it's safe (and necessary) to relabel these even if
> relabeling is disabled and thus we can change the function to always
> label FD's.
> Do you agree?
Yeah, it looks that way to me. All this relabel=no stuff was basically
there to cope with files on NFS volumes. So the key thing is to make
sure that with whatever change you make, the APIs above all work
correctly if the file path points to something on NFS.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|