On Wed, 05 Mar 2008 09:11:10 -0500 Eric Paris <eparis@xxxxxxxxxx> wrote: > > On Wed, 2008-03-05 at 08:48 -0500, Jeff Layton wrote: > > On Mon, 03 Mar 2008 14:42:52 -0500 > > Eric Paris <eparis@xxxxxxxxxx> wrote: > > > > > In the current code (approved by SELinux and NFS people in 2004) SELinux > > > tries to understand NFS's binary mount data. This blows up in the face > > > of things like nohide mounts which don't use struct nfs_mount_data and I > > > assume just looking at the code that things don't work since NFS moved > > > to using nfs_parsed_mount_data as its default binary mount data blob. > > > > > > This patch creates a new LSM interfaces allowing NFS to hand argument > > > processing over to the LSM and get back a binary blob the LSM > > > understands for later user. There is no specific SELinux knowledge > > > inside NFS and no NFS information left inside SELinux. NFS now passes > > > either strings or the LSM data blob into the LSM and lets the LSM do its > > > own work. This means that all LSM mount options are supported by NFS > > > when it uses the text userspace interface. > > > > > > This patch makes NFS use the new LSM hooks security_sb_set_mnt_opts() > > > and security_sb_clone_mnt_opts() inside the ->get_sb() calls so that > > > security options are set explicitly by NFS before the generic NFS > > > settings used by non binary mount data fs's. The only sorta > > > 'selinux-ism' in this patch is for the NFS binary mount data interface > > > (which must keep working to stop any regressions) to reattach "context=" > > > to the data. This part of the string will have been removed by the > > > older mount.nfs userspace parser. That little nugget is well commented > > > and wrapper in CONFIG_SECURITY_SELINUX. > > > > > > We need this for 2.6.25 since at the moment SELinux (and SMACK) + nohide > > > mounts cause security_sb_copy_mount_data() to copy one page of mount > > > data starting at the struct nfs_clone_mount_data on the stack. If the > > > stack doesn't span 2 pages we run off the end of the stack and hit a > > > page fault BUG(). This also solves the regression in functionality > > > since all SELinux support was broken with the switch to > > > nfs_parsed_mount_data. > > > > > > Signed-off-by: Eric Paris <eparis@xxxxxxxxxx> > > > > > > --- > > > > > > This patch was tested with NFSv3 mounts and solves all of the > > > regressions that have been introduced in the NFS code. It supports both > > > the binary interface and text options without pushing LSM specific > > > parsing into the FS. It doesn't introduce the new LSM constructs > > > necessary to show these mount options in /proc/mounts suggested by > > > Miklos Szeredi but that will be coming in 2.6.26 (it's not a regression > > > or bug fix). I also do not have the proper security_sb_set_mnt_opts() > > > call in nfs4_get_sb due to a lack of a testing environment, but will add > > > that in 2.6.26 as well. SELinux + NFSv4 have never had context mount > > > options so there is no regression nor loss of functionality. > > > > > > Would the NFS community like to push all of this through their bug fix > > > trees or would they prefer I just push this whole patch up the SELinux > > > trees? > > > > > > > I like the basic approach -- seems like a clean way to do this. I'm > > not sure I know enough about the selinux/security internals to give > > that a good review, though I made a quick pass through it. Which brings > > me to a somewhat minor nit... > > > > Would it be possible to break this patch up while keeping the tree > > cleanly bisectable? Maybe one or more patches that add the new > > interfaces and then another separate NFS patch that changes it to use > > the new interfaces? Not only would that make this easier to review, but > > the separate NFS patch might also help serve as a model for other > > filesystems that want to take advantage of the new interfaces. > > Yes, I can cleanly break the fs/nfs/internal.h and fs/nfs/super.c > changes into a seperate patch. It doesn't really help reviewability > though since they are already clearly segregated. Originally I was > keeping around the legacy support for git biscect reasons but the switch > to nfs_parsed_mount_data already broke everything so there wouldn't be a > lose to separating them. I'll send as 2 patches in a moment. > > > > @@ -589,6 +582,21 @@ static int selinux_set_mnt_opts(struct super_block *sb, char **mount_options, > > > } > > > > > > /* > > > + * Binary mount data FS will come through this function twice. Once > > > + * from an explicit call and once from the generic calls from the vfs. > > > + * Since the generic VFS calls will not contain any security mount data > > > + * we need to skip the double mount verification. > > > + * > > > + * This does open a hole in which we will not notice if the first > > > + * mount using this sb set explict options and a second mount using > > > + * this sb does not set any security options. (The first options > > > + * will be used for both mounts) > > > + */ > > > + if (sbsec->initialized && (sb->s_type->fs_flags & FS_BINARY_MOUNTDATA) > > > + && (num_opts == 0)) > > > + goto out; > > > + > > > > I'm not sure I understand this. What purpose does checking num_opts > > serve here? > > > > > + /* > > > * parse the mount options, check if they are valid sids. > > > * also check if someone is trying to mount the same sb more > > > * than once with different security options. > > Let me explain the 2 mount paths and all the logic going on in here for > the case of NFS. It is the only interesting FS. > > The two call paths to get here both come through vfs_kern_mount() > > vfs_kern_mount() > get_sb() > nfs_get_sb() > security_sb_set_mnt_opts() > > vfs_kern_mount() > security_sb_kern_mount() > superblock_doinit() > security_sb_set_mnt_opts() > > Now in this function we do multiple things (even through the name only > implies one thing.) 1) We set the security options on the superblock > security structure and 2) we make sure that these options don't conflict > with options previously used. > > The idea is that some someone might do > > mount -o context=context1 /dev/whatever /mnt/whatever1 > mount -o context=context2 /dev/whatever /mnt/whatever2 > > This is going to use the same superblock but the context= needs to the > same. There is no was to reconcile the 2, so we just reject the second > mount. > We could just not share superblocks in that case. Maybe add a new condition to nfs_compare_mount_options()? When that returns 0 now, I believe we spin off a new superblock. > Binary FS's like NFS which call into the explicit set function will > still traverse the generic call path. But on the generic call patch > vfs_kern_mount() is going to pass in null mount data and > superblock_doinit is going to turn that into an empty structure with > num_opts=0. We don't want the mount to fail here because the mount > options set explicitly a moment ago aren't the same now. Solutions > meant I either needed to be able to tell that it was the same mount > operation we saw on this code path both time or to just let it through > the second time. I decided to just allow it through. > > The drawback of the approach I took is that someone who does > > mount -o context=context1 server1:/export/ /mnt/whatever1 > mount server1:/export/ /mnt/whatever2 > > is NOT going to get a failure on the second mount. It will still mount > just fine and it is going to share the mount options with the first > mount, just the user is not going to be informed that the security > options given were not the same on both. > > Too much info? was I just babbling? did it make sense? Let me know if > you still have any questions or problems with the possible outcomes... > I think it makes sense. We already have some infrastructure to spawn new superblocks when we have differing mount options. It seems like different context= options should behave the same way. Have a look at nfs_compare_mount_options(). If you make it so that that returns 0 when the context= options don't match then I think the new mount will get a new sb, and you'll be able to apply the new context= option to it. -- Jeff Layton <jlayton@xxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
