Re: [PATCH 1/9] Unionfs: security convert lsm into a static interface fix

[Erez Zadok <ezk@cs.sunysb.edu>]

In message <20071023090739.GA19758@infradead.org>, Christoph Hellwig writes:
> On Mon, Oct 22, 2007 at 08:48:04PM -0400, Erez Zadok wrote:
> > Why?  Are you concerned that the security policy may change after a module
> > is loaded?
> 
> No, it's a matter of proper layering.  We generally don't want modules
> like stackabke filesystems to call directly into methods but rather use
> proper highlevel VFS helpers to isolate them from details and possible
> changes.  The move to out of line security_ helpers just put this on the
> radard.

OK, I'll be shortly posting a couple of patches to fs/ioctl.c.

> > I can probably get rid of having unionfs call security_inode_permission,
> > by calling permission() myself and carefully post-process its return
> > code (unionfs needs to "ignore" EROFS initially, to allow copyup to take
> > place).
> 
> Sounds fine.

I was able to test this idea and it works fine.  Now unionfs calls
permission(), post-processes the return value, and I don't need my own
modified version of permission() in unionfs.  This saved me ~50 LoC and
reduced stack pressure a little.

> > But security_file_ioctl doesn't have any existing helper I can call.  I
> > can introduce a trivial vfs_security_file_ioctl wrapper to
> > security_file_ioctl, but what about the already existing *19* exported
> > security_* functions in security/security.c?  Do you want to see simple
> > wrappers for all of them?  It seems redundant to add a one-line wrapper
> > around an already one-line function around security_ops->XXX.  Plus,
> > some of the existing exported security_* functions are file-system
> > related, others are networking, etc.  So we'll need wrappers whose names
> > are prefixed appropriately: vfs_*, net_*, etc.
> 
> The fix for security_file_ioctl is probably to either not do it at all
> or move it the call to security_file_ioctl into vfs_ioctl and get it by
> using that helper.  I suspect most other security_ exports should be
> avoided similarly.

Christoph, I looked more closely at that and the selinux code.  Only
sys_ioctl calls security_file_ioctl.  And security_file_ioctl performs all
sorts of checks that mostly have to do with the currently running task or
the open file.  The running task is still the same, whether filesystem
stacking is involved or not.  Also, the unionfs-level struct file is
logically the same file at the lower level: they refer to the same object,
just at two layers.  I can't see any reason why unionfs_ioctl should have to
call security_file_ioctl(lower_file) the way sys_ioctl does: that check is
already done well before the file system's ->ioctl is invoked.  I also don't
see how it would ever be possible that sys_ioctl will succeed in its call to
security_file_ioctl(upper_file), but unionfs will fail the same security
check on the lower file.

So I commented out unionfs's call to security_file_ioctl(lower_file) and
tested it on a bunch of things, including an selinux-enabled livecd.
Everything seemed to work just fine, so I'll be sending some patches to that
effect, and we can drop the -mm patch which exports security_file_ioctl().
BTW, ecryptfs doesn't call security_file_ioctl().

Cheers,
Erez.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/