Re: libselinux mountpoint changing patch.

[Daniel J Walsh <dwalsh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3345 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/03/2011 11:33 AM, Stephen Smalley wrote:
> On Tue, 2011-05-03 at 10:50 -0400, Daniel J Walsh wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> The Fedora Distribution is looking to standardize kernel subsystem file
>> systems to be mounted under /sys/fs. They would like us to move /selinux
>> to /sys/fs/selinux.  This patch changes libselinux in the following ways:
>>
>> 1.  load_policy will first check if /sys/fs/selinux exists and mount the
>> selinuxfs at this location, if it does not exists it will fall back to
>> mounting the file system at /selinux (if it exists).
>>
>> 2.  The init functions of selinux will now check if /sys/fs/selinux is
>> mounted, if it is and has an SELinuxfs mounted on it, the code will then
>> check if the selinuxfs is mounted rw, if it is, libselinux will set the
>> mountpoint, if it is readonly, libselinux will return no mountpoint.  If
>> /sys/fs/selinux does not exists, the same check will be done for
>> /selinux and finally for an entry in /proc/mounts.
>>
>> NOTE:  We added the check for RO, to allow tools like mock to be able to
>> tell a chroot that SELinux is disabled while enforcing it outside the
>> chroot.
>>
>>
>> # getenforce
>> Enabled
>> # mount -t selinuxfs -o remount,ro selinuxfs /var/chroot/selinux
> 
> Just to clarify, the right commands to use are:
> mount --bind /selinux /var/chroot/selinux
> mount -o remount,ro /var/chroot/selinux
> 
> Do not use:
> mount -t selinuxfs -o ro selinuxfs /var/chroot/selinux
> as this will in fact change the flags on /selinux as well.  Surprise!
> Result of there only being a single instance (superblock) of selinuxfs,
> although you can have multiple vfsmounts of it.
> 
>> # chroot /var/chroot
>> # getenforce
>> Disabled
>>
>> 3. In order to make this work, I needed to stop enabled from checking if
>> /proc/filesystem for entries if selinux_mnt did not exist.  Now enabeled
>> checks if selinux_mnt has been discovered otherwise it will report
>> selinux disabled.
> 
> Looks reasonable, minor comments below.
> 
> Can we really not get all the necessary information from a single call
> (as opposed to having to call both statfs() and statvfs())?  Isn't
> statvfs() implemented on Linux by calling the statfs system call?
> 
Not that I can see.

> I'd suggest adding a #define OLDSELINUXMNT "/selinux" to policy.h and
> using OLDSELINUXMNT in init.c and load_policy.c rather than sprinkling
> "/selinux" around multiple places.   Wouldn't hurt to #define SELINUXFS
> "selinuxfs" as well and replacing all occurrences in init.c and
> load_policy.c.
> 
Ok
> As check_mountpoint() sets selinux_mnt, I'd pick a more descriptive
> name.  Actually, could you perhaps fold the logic into set_selinuxmnt()?
> That would mean the validation would happen when set_selinuxmnt() gets
> called by load_policy, which isn't strictly necessary but does no harm.
> 

Done

I have to change set_selinuxmnt to return an int now, though.
Does this mean we would need an API version bump?  Changing from void
return to int?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk3AJ28ACgkQrlYvE4MpobPMBwCghY08MsDjpufL/NPkFWfC7M6v
9kgAoI8Gi0Z0LROlxPYgtvcShmZkLEKb
=4NO/
-----END PGP SIGNATURE-----

[-- Attachment #2: libselinux-mountpoint.patch --]
[-- Type: text/plain, Size: 6219 bytes --]

diff --git a/libselinux/include/selinux/selinux.h b/libselinux/include/selinux/selinux.h
index f110dcf..644d6d2 100644
--- a/libselinux/include/selinux/selinux.h
+++ b/libselinux/include/selinux/selinux.h
@@ -513,7 +513,7 @@ extern int selinux_check_securetty_context(const security_context_t tty_context)
    Normally, this is determined automatically during libselinux 
    initialization, but this is not always possible, e.g. for /sbin/init
    which performs the initial mount of selinuxfs. */
-void set_selinuxmnt(char *mnt);
+int set_selinuxmnt(char *mnt);
 
 /* clear selinuxmnt variable and free allocated memory */
 void fini_selinuxmnt(void);
diff --git a/libselinux/src/enabled.c b/libselinux/src/enabled.c
index b3c8c47..018c787 100644
--- a/libselinux/src/enabled.c
+++ b/libselinux/src/enabled.c
@@ -11,10 +11,6 @@
 
 int is_selinux_enabled(void)
 {
-	char *buf=NULL;
-	FILE *fp;
-	ssize_t num;
-	size_t len;
 	int enabled = 0;
 	security_context_t con;
 
@@ -32,37 +28,8 @@ int is_selinux_enabled(void)
 				enabled = 0;
 			freecon(con);
 		}
-		return enabled;
         }
 
-	/* Drop back to detecting it the long way. */
-	fp = fopen("/proc/filesystems", "r");
-	if (!fp)
-		return -1;
-
-	__fsetlocking(fp, FSETLOCKING_BYCALLER);
-	while ((num = getline(&buf, &len, fp)) != -1) {
-		if (strstr(buf, "selinuxfs")) {
-			enabled = 1;
-			break;
-		}
-	}
-
-	if (num < 0)
-		goto out;
-
-	/* Since an selinux file system is available, we consider
-	 * selinux enabled. If getcon_raw fails, selinux is still
-	 * enabled. We only consider it disabled if no policy is loaded. */
-	if (getcon_raw(&con) == 0) {
-		if (!strcmp(con, "kernel"))
-			enabled = 0;
-		freecon(con);
-	}
-
-      out:
-	free(buf);
-	fclose(fp);
 	return enabled;
 }
 
diff --git a/libselinux/src/init.c b/libselinux/src/init.c
index a948920..547f1eb 100644
--- a/libselinux/src/init.c
+++ b/libselinux/src/init.c
@@ -7,6 +7,7 @@
 #include <stdio.h>
 #include <stdio_ext.h>
 #include <dlfcn.h>
+#include <sys/statvfs.h>
 #include <sys/vfs.h>
 #include <stdint.h>
 #include <limits.h>
@@ -24,8 +25,6 @@ static void init_selinuxmnt(void)
 {
 	char *buf=NULL, *p;
 	FILE *fp=NULL;
-	struct statfs sfbuf;
-	int rc;
 	size_t len;
 	ssize_t num;
 	int exists = 0;
@@ -33,17 +32,9 @@ static void init_selinuxmnt(void)
 	if (selinux_mnt)
 		return;
 
-	/* We check to see if the preferred mount point for selinux file
-	 * system has a selinuxfs. */
-	do {
-		rc = statfs(SELINUXMNT, &sfbuf);
-	} while (rc < 0 && errno == EINTR);
-	if (rc == 0) {
-		if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
-			selinux_mnt = strdup(SELINUXMNT);
-			return;
-		}
-	} 
+	if (set_selinuxmnt(SELINUXMNT) == 0) return;
+
+	if (set_selinuxmnt(OLDSELINUXMNT) == 0) return;
 
 	/* Drop back to detecting it the long way. */
 	fp = fopen("/proc/filesystems", "r");
@@ -52,7 +43,7 @@ static void init_selinuxmnt(void)
 
 	__fsetlocking(fp, FSETLOCKING_BYCALLER);
 	while ((num = getline(&buf, &len, fp)) != -1) {
-		if (strstr(buf, "selinuxfs")) {
+		if (strstr(buf, SELINUXFS)) {
 			exists = 1;
 			break;
 		}
@@ -79,7 +70,7 @@ static void init_selinuxmnt(void)
 		tmp = strchr(p, ' ');
 		if (!tmp)
 			goto out;
-		if (!strncmp(tmp + 1, "selinuxfs ", 10)) {
+		if (!strncmp(tmp + 1, SELINUXFS, 10)) {
 			*tmp = '\0';
 			break;
 		}
@@ -87,7 +78,7 @@ static void init_selinuxmnt(void)
 
 	/* If we found something, dup it */
 	if (num > 0)
-		selinux_mnt = strdup(p);
+		set_selinuxmnt(p);
 
       out:
 	free(buf);
@@ -104,9 +95,30 @@ void fini_selinuxmnt(void)
 
 hidden_def(fini_selinuxmnt)
 
-void set_selinuxmnt(char *mnt)
+int set_selinuxmnt(char *mnt)
 {
-	selinux_mnt = strdup(mnt);
+	struct statfs sfbuf;
+	int rc;
+
+	/* We check to see if the preferred mount point for selinux file
+	 * system has a selinuxfs. */
+	do {
+		rc = statfs(mnt, &sfbuf);
+	} while (rc < 0 && errno == EINTR);
+	if (rc == 0) {
+		if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
+			struct statvfs vfsbuf;
+			rc = statvfs(mnt, &vfsbuf);
+			if (rc == 0) {
+				if (!(vfsbuf.f_flag & ST_RDONLY)) {
+					selinux_mnt = strdup(mnt);
+					return 0;
+				}
+			}
+		}
+	} 
+
+	return -1;
 }
 
 hidden_def(set_selinuxmnt)
diff --git a/libselinux/src/load_policy.c b/libselinux/src/load_policy.c
index 83d2143..f6eae49 100644
--- a/libselinux/src/load_policy.c
+++ b/libselinux/src/load_policy.c
@@ -369,7 +369,17 @@ int selinux_init_load_policy(int *enforce)
 	 * Check for the existence of SELinux via selinuxfs, and 
 	 * mount it if present for use in the calls below.  
 	 */
-	if (mount("selinuxfs", SELINUXMNT, "selinuxfs", 0, 0) < 0 && errno != EBUSY) {
+	char *mntpoint = NULL;
+	if (mount(SELINUXFS, SELINUXMNT, SELINUXFS, 0, 0) == 0 || errno == EBUSY) {
+		mntpoint = SELINUXMNT;
+	} else { 
+		/* check old mountpoint */
+		if (mount(SELINUXFS, OLDSELINUXMNT, SELINUXFS, 0, 0) == 0 || errno == EBUSY) {
+			mntpoint = OLDSELINUXMNT;
+		}
+	} 
+
+	if (! mntpoint ) {
 		if (errno == ENODEV) {
 			/*
 			 * SELinux was disabled in the kernel, either
@@ -384,8 +394,11 @@ int selinux_init_load_policy(int *enforce)
 		}
                 
 		goto noload;
+	} 
+	if (set_selinuxmnt(mntpoint) != 0) {
+		fprintf(stderr, "Mount failed for selinuxfs on %s:  %s\n", mntpoint, strerror(errno));
+		goto noload;
 	}
-	set_selinuxmnt(SELINUXMNT);
 
 	/*
 	 * Note:  The following code depends on having selinuxfs 
@@ -397,7 +410,7 @@ int selinux_init_load_policy(int *enforce)
 		rc = security_disable();
 		if (rc == 0) {
 			/* Successfully disabled, so umount selinuxfs too. */
-			umount(SELINUXMNT);
+			umount(selinux_mnt);
 			fini_selinuxmnt();
 		}
 		/*
diff --git a/libselinux/src/policy.h b/libselinux/src/policy.h
index 10e8712..bf270b5 100644
--- a/libselinux/src/policy.h
+++ b/libselinux/src/policy.h
@@ -9,11 +9,15 @@
 /* Initial length guess for getting contexts. */
 #define INITCONTEXTLEN 255
 
+/* selinux file system type */
+#define SELINUXFS "selinuxfs"
+
 /* selinuxfs magic number */
 #define SELINUX_MAGIC 0xf97cff8c
 
 /* Preferred selinux mount location */
-#define SELINUXMNT "/selinux"
+#define SELINUXMNT "/sys/fs/selinux"
+#define OLDSELINUXMNT "/selinux"
 
 /* selinuxfs mount point */
 extern char *selinux_mnt;

[-- Attachment #3: libselinux-mountpoint.patch.sig --]
[-- Type: application/pgp-signature, Size: 72 bytes --]