Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Martin Kletzander 
On 10/15/2012 09:12 AM, Guannan Ren wrote:
 If we use matchpathcon() to look up selinux context for specific pathname,
 it'd better actively load file contexts database by matchpathcon_init()
 and free memory when finished using matchpathcon by matchpathcon_fini().
 ---
  src/security/security_selinux.c | 8 
  1 file changed, 8 insertions(+)
 
 diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
 index 10135ed..b278e2c 100644
 --- a/src/security/security_selinux.c
 +++ b/src/security/security_selinux.c
 @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
 *virtDriver)
  static int
  virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
  {
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_init(NULL)  0)
 +VIR_WARN(cannot load selinux active file contexts configuration);
 +#endif
  return virSecuritySELinuxInitialize(mgr);
  }
  
 @@ -685,6 +689,10 @@ 
 virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
  VIR_FREE(data-file_context);
  VIR_FREE(data-content_context);
  
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_fini()  0)
 +VIR_WARN(cannot free allocated memory for selinux);
 +#endif
  return 0;
  }
  
 

ACK,

Martin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Daniel P. Berrange 
On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
 If we use matchpathcon() to look up selinux context for specific pathname,
 it'd better actively load file contexts database by matchpathcon_init()
 and free memory when finished using matchpathcon by matchpathcon_fini().
 ---
  src/security/security_selinux.c | 8 
  1 file changed, 8 insertions(+)
 
 diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
 index 10135ed..b278e2c 100644
 --- a/src/security/security_selinux.c
 +++ b/src/security/security_selinux.c
 @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
 *virtDriver)
  static int
  virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
  {
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_init(NULL)  0)
 +VIR_WARN(cannot load selinux active file contexts configuration);
 +#endif
  return virSecuritySELinuxInitialize(mgr);
  }
  
 @@ -685,6 +689,10 @@ 
 virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
  VIR_FREE(data-file_context);
  VIR_FREE(data-content_context);
  
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_fini()  0)
 +VIR_WARN(cannot free allocated memory for selinux);
 +#endif
  return 0;
  }

I'm not convinced this is safe, because the security drivers can be
opened multiple times, eg LXC and QEMU, and this is changing the global
static state of the SELinux library.



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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Martin Kletzander 
On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
 On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
 If we use matchpathcon() to look up selinux context for specific pathname,
 it'd better actively load file contexts database by matchpathcon_init()
 and free memory when finished using matchpathcon by matchpathcon_fini().
 ---
  src/security/security_selinux.c | 8 
  1 file changed, 8 insertions(+)

 diff --git a/src/security/security_selinux.c 
 b/src/security/security_selinux.c
 index 10135ed..b278e2c 100644
 --- a/src/security/security_selinux.c
 +++ b/src/security/security_selinux.c
 @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
 *virtDriver)
  static int
  virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
  {
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_init(NULL)  0)
 +VIR_WARN(cannot load selinux active file contexts configuration);
 +#endif
  return virSecuritySELinuxInitialize(mgr);
  }
  
 @@ -685,6 +689,10 @@ 
 virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
  VIR_FREE(data-file_context);
  VIR_FREE(data-content_context);
  
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_fini()  0)
 +VIR_WARN(cannot free allocated memory for selinux);
 +#endif
  return 0;
  }
 
 I'm not convinced this is safe, because the security drivers can be
 opened multiple times, eg LXC and QEMU, and this is changing the global
 static state of the SELinux library.
 

I didn't think the driver is opened for every other driver used.  In
this case the initialization of the matchpathcon should be dealt with in
some other way.  Or can't we open the security driver only once?

@Guannan: is there a problem this fixes?  How come it worked without the
init before?

Martin

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Daniel P. Berrange 
On Mon, Oct 15, 2012 at 12:40:56PM +0200, Martin Kletzander wrote:
 On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
  On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
  If we use matchpathcon() to look up selinux context for specific pathname,
  it'd better actively load file contexts database by matchpathcon_init()
  and free memory when finished using matchpathcon by matchpathcon_fini().
  ---
   src/security/security_selinux.c | 8 
   1 file changed, 8 insertions(+)
 
  diff --git a/src/security/security_selinux.c 
  b/src/security/security_selinux.c
  index 10135ed..b278e2c 100644
  --- a/src/security/security_selinux.c
  +++ b/src/security/security_selinux.c
  @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
  *virtDriver)
   static int
   virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
   {
  +#ifndef HAVE_SELINUX_LABEL_H
  +if (matchpathcon_init(NULL)  0)
  +VIR_WARN(cannot load selinux active file contexts 
  configuration);
  +#endif
   return virSecuritySELinuxInitialize(mgr);
   }
   
  @@ -685,6 +689,10 @@ 
  virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
   VIR_FREE(data-file_context);
   VIR_FREE(data-content_context);
   
  +#ifndef HAVE_SELINUX_LABEL_H
  +if (matchpathcon_fini()  0)
  +VIR_WARN(cannot free allocated memory for selinux);
  +#endif
   return 0;
   }
  
  I'm not convinced this is safe, because the security drivers can be
  opened multiple times, eg LXC and QEMU, and this is changing the global
  static state of the SELinux library.
  
 
 I didn't think the driver is opened for every other driver used.  In
 this case the initialization of the matchpathcon should be dealt with in
 some other way.  Or can't we open the security driver only once?

I say we ignore use of matchpathcon_fini(), and simply call
matchpathcon_init() from a VIR_GLOBAL_INIT macro

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Guannan Ren 

On 10/15/2012 06:40 PM, Martin Kletzander wrote:

On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:

On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:

If we use matchpathcon() to look up selinux context for specific pathname,
it'd better actively load file contexts database by matchpathcon_init()
and free memory when finished using matchpathcon by matchpathcon_fini().
---
  src/security/security_selinux.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 10135ed..b278e2c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
*virtDriver)
  static int
  virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
  {
+#ifndef HAVE_SELINUX_LABEL_H
+if (matchpathcon_init(NULL)  0)
+VIR_WARN(cannot load selinux active file contexts configuration);
+#endif
  return virSecuritySELinuxInitialize(mgr);
  }
  
@@ -685,6 +689,10 @@ virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)

  VIR_FREE(data-file_context);
  VIR_FREE(data-content_context);
  
+#ifndef HAVE_SELINUX_LABEL_H

+if (matchpathcon_fini()  0)
+VIR_WARN(cannot free allocated memory for selinux);
+#endif
  return 0;
  }

I'm not convinced this is safe, because the security drivers can be
opened multiple times, eg LXC and QEMU, and this is changing the global
static state of the SELinux library.


I didn't think the driver is opened for every other driver used.  In
this case the initialization of the matchpathcon should be dealt with in
some other way.  Or can't we open the security driver only once?

@Guannan: is there a problem this fixes?  How come it worked without the
init before?

Martin



   No, there is no a bug/problem to fix, the patch just actively 
initializes

   the active context configuration database.
   If we don't do this, the matchpathcon will do it instead.
   I am going to add them in a better way later.
   This patch has nothing to do with the 2/2 patch.

   Guannan

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Daniel P. Berrange 
On Mon, Oct 15, 2012 at 07:03:07PM +0800, Guannan Ren wrote:
 On 10/15/2012 06:40 PM, Martin Kletzander wrote:
 On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:
 On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:
 If we use matchpathcon() to look up selinux context for specific pathname,
 it'd better actively load file contexts database by matchpathcon_init()
 and free memory when finished using matchpathcon by matchpathcon_fini().
 ---
   src/security/security_selinux.c | 8 
   1 file changed, 8 insertions(+)
 
 diff --git a/src/security/security_selinux.c 
 b/src/security/security_selinux.c
 index 10135ed..b278e2c 100644
 --- a/src/security/security_selinux.c
 +++ b/src/security/security_selinux.c
 @@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
 *virtDriver)
   static int
   virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
   {
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_init(NULL)  0)
 +VIR_WARN(cannot load selinux active file contexts 
 configuration);
 +#endif
   return virSecuritySELinuxInitialize(mgr);
   }
 @@ -685,6 +689,10 @@ 
 virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
   VIR_FREE(data-file_context);
   VIR_FREE(data-content_context);
 +#ifndef HAVE_SELINUX_LABEL_H
 +if (matchpathcon_fini()  0)
 +VIR_WARN(cannot free allocated memory for selinux);
 +#endif
   return 0;
   }
 I'm not convinced this is safe, because the security drivers can be
 opened multiple times, eg LXC and QEMU, and this is changing the global
 static state of the SELinux library.
 
 I didn't think the driver is opened for every other driver used.  In
 this case the initialization of the matchpathcon should be dealt with in
 some other way.  Or can't we open the security driver only once?
 
 @Guannan: is there a problem this fixes?  How come it worked without the
 init before?
 
 Martin
 
 
No, there is no a bug/problem to fix, the patch just actively
 initializes
the active context configuration database.
If we don't do this, the matchpathcon will do it instead.
I am going to add them in a better way later.

There *is* a bug, because you are calling the init/fini methods multiple
times and the libselinux code does not handle that usage in a safe manner.

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 1/2] selinux: load and free selinux active file contexts configuration database

2012-10-15 Thread Guannan Ren 

On 10/15/2012 07:35 PM, Daniel P. Berrange wrote:

On Mon, Oct 15, 2012 at 07:03:07PM +0800, Guannan Ren wrote:

On 10/15/2012 06:40 PM, Martin Kletzander wrote:

On 10/15/2012 12:22 PM, Daniel P. Berrange wrote:

On Mon, Oct 15, 2012 at 03:12:45PM +0800, Guannan Ren wrote:

If we use matchpathcon() to look up selinux context for specific pathname,
it'd better actively load file contexts database by matchpathcon_init()
and free memory when finished using matchpathcon by matchpathcon_fini().
---
  src/security/security_selinux.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 10135ed..b278e2c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -667,6 +667,10 @@ virSecuritySELinuxSecurityDriverProbe(const char 
*virtDriver)
  static int
  virSecuritySELinuxSecurityDriverOpen(virSecurityManagerPtr mgr)
  {
+#ifndef HAVE_SELINUX_LABEL_H
+if (matchpathcon_init(NULL)  0)
+VIR_WARN(cannot load selinux active file contexts configuration);
+#endif
  return virSecuritySELinuxInitialize(mgr);
  }
@@ -685,6 +689,10 @@ 
virSecuritySELinuxSecurityDriverClose(virSecurityManagerPtr mgr)
  VIR_FREE(data-file_context);
  VIR_FREE(data-content_context);
+#ifndef HAVE_SELINUX_LABEL_H
+if (matchpathcon_fini()  0)
+VIR_WARN(cannot free allocated memory for selinux);
+#endif
  return 0;
  }

I'm not convinced this is safe, because the security drivers can be
opened multiple times, eg LXC and QEMU, and this is changing the global
static state of the SELinux library.


I didn't think the driver is opened for every other driver used.  In
this case the initialization of the matchpathcon should be dealt with in
some other way.  Or can't we open the security driver only once?

@Guannan: is there a problem this fixes?  How come it worked without the
init before?

Martin


No, there is no a bug/problem to fix, the patch just actively
initializes
the active context configuration database.
If we don't do this, the matchpathcon will do it instead.
I am going to add them in a better way later.

There *is* a bug, because you are calling the init/fini methods multiple
times and the libselinux code does not handle that usage in a safe manner.

Daniel


Yes, I learned this from your comments in other emails
I means this patch is not to fix particular existing BZ.
I will try to add init/fini in a global way later. :)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list