From: Eric Paris <eparis@redhat.com> Subject: [RHEL5 PATCH] BZ 219837 allow NFS nohide and SELinux to work together Date: Wed, 06 Jun 2007 15:46:25 -0400 Bugzilla: 219837 Message-Id: <1181159185.3776.32.camel@dhcp231-215.rdu.redhat.com> Changelog: [security] allow NFS nohide and SELinux to work together BZ 219837 This is a terrible aweful ugly hack (I could say more, but I'll let the patch/description speak for itself). Lets just get that out of the way up front. I'm working with upstream for a real solution which is going to involve new security hooks and things like that, but it just isn't ready for 5.1. This patch is safe and will work, but I recognize its gross. If possible I'll try to get the upstream solution back for U2 rather than carry this nastiness forever. *** When selinux attempts to do its part of mounting a new FS it checks the mount data for selinux context information. For most FS's this is a text string but for NFS this is a data blob (struct nfs_mount_data) Version 6 and later of this struct have a context field which selinux can use to pass context information from userspace to where it is used by selinux (security/selinux/hooks.c:try_context_mount()) The problem here is that we have found that exports with the 'nohide' option actually get mounted from fs/nfs/namespace.c:nfs_do_submount() which uses a struct nfs_clone_mount for its data. This gets passed down into the selinux code nfs_do_submount() nfs_do_clone_mount() vfs_kern_mount() security_sb_kern_mount() selinux_sb_kern_mount() superblock_doinit() try_context_mount() which finds that the fs being mounted is nfs based on: strcmp(sb->s_type->name, "nfs") The code in try_context_mount then casts the data to a struct nfs_mount_data. Notice I just said the code cast a nfs_clone_mount to a nfs_mount_data. Next it checks the version to make sure it should have the context field. If so it uses the value in the context field if not it goes on. The problem comes that the first entry in the nfs_mount_data is the version but the first entry in the nfs_clone_mount is a pointer. Since the pointer is always bigger than the version we need we then try to dereference ->context which is outside of the data blob. Since nfs_clone_data is smaller in size than nfs_mount_data we are just getting some stuff off of the stack. If that data happens to be 0 (has been that way on all of my x86ish systems) everything works just fine. If that data is not 0 the selinux code gets angry and refuses to mount the FS (which happens for me on other arches like ia64). This gross hack continues the bad cast, and continues to look at ->version, but checks if it is too big as well as too small. On every arch that we deal with no pointer to a kernel data struct would be > 6 and < 56 that I know of. I tested this patch on a number of arches and everything seems to be working happily. Although I admit that the only code path I know how to exercise is through nfs_do_submount and I don't know how to exercise the nfs_follow_referal code path or even exactly what implications referals have on this issue today. I ask steved if his referrals patches had selinux on, but I forgot that x86ish arches seemed to always get 0 for the data->context and not have a problem even with the broken logic. My guess is that referals are also busted on ia64, but just a guess. This should fix that issue no matter what. I plan to get the real fix upstream and I'll pull it into 5.2 if possible, but for now I think the gross hack is actually not a bad way to go. What do others think? -Eric --- linux-2.6.18.i686/security/selinux/hooks.c.pre.nfs.nohide.hack 2007-06-06 20:04:53.000000000 -0400 +++ linux-2.6.18.i686/security/selinux/hooks.c 2007-06-06 20:16:48.000000000 -0400 @@ -384,7 +384,25 @@ static int try_context_mount(struct supe if (!strcmp(name, "nfs")) { struct nfs_mount_data *d = data; - if (d->version < NFS_MOUNT_VERSION) + /* + * ***FIX ME**** This is a terrible ugly ugly hack. + * when try_context_mount comes from changing FS on the + * server because of nohide (or maybe a referal??) the + * data is actually struct nfs_clone_mount and so that + * cast is wrong. bit for bit the version field from + * *data lines up with a pointer in the other possible + * structure. This hack will make sure the ->version + * field looks like a version rather than a pointer. + * If its a pointer bail cleanly and let things keep + * working. Should get fixed correctly upstream but the + * issue isn't simple or straightforward. + * + * There have only been 6 nfs_mount_data versions so far + * so if over one update we increate the version 50 times + * I think we have other problems..... + */ + if (d->version < NFS_MOUNT_VERSION || + d->version > NFS_MOUNT_VERSION + 50) goto out; if (d->context[0]) {