Re: [PATCH] Security/sysfs: Enable security xattrs to be set onsysfs files, directories, and symlinks.

[David P. Quigley]

On Thu, 2009-07-09 at 07:49 -0700, Casey Schaufler wrote:
> David P. Quigley wrote:
> > On Wed, 2009-07-08 at 18:44 -0700, Casey Schaufler wrote:
> >   
> >> David P. Quigley wrote:
> >>     
> >>> This patch adds a setxattr handler to the file, directory, and symlink
> >>> inode_operations structures for sysfs. This handler uses two new LSM hooks. The
> >>> first hook takes the xattr name and value and turns the context into a secid.
> >>> This is embedded into the sysfs_dirent structure so it remains persistent even
> >>> if the inode structures are evicted from the cache. The second hook allows for
> >>> the secid to be taken from the sysfs_dirent and be pushed into the inode
> >>> structure as the actual secid for the inode.
> >>>   
> >>>       
> >> Nacked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> >>
> >> I'm all for sysfs supporting xattrs.
> >>
> >> I am completely opposed to secids as file system metadata.
> >>
> >> What do you get when you do an ls -Z?
> >>
> >> An LSM must not be beholden to exposing transient internal
> >> representations of security data to userspace, which is what
> >> you're doing here. An LSM gets to decide what the security
> >> information it maintains looks like by defining a security blob.
> >>
> >> If you want this in, implement xattrs in sysfs for real. Smack
> >> depends on the existing, published, and supported xattr interfaces
> >> for dealing with getting and setting the values. Not secids.
> >> Smack maintains secids because labeled networking and audit require
> >> them, and they got there first.
> >>
> >>
> >>     
> >
> > So are you proposing that we embed a variable length string in the
> > sysfs_dirent structure because that sounds completely silly. 
> 
> No, I'm not proposing that because it sounds silly, I'm proposing it
> because that's the way xattrs work on Linux.
> 
> > It seems
> > completely reasonable here to take the blob coming in and have the LSM
> > turn it into a handle that is efficiently referenced by the
> > sysfs_dirent. The problem here is that sysfs entries have no backing
> > store at all which means everything we do will have to be added to
> > sysfs_dirent. I'm pretty sure we don't want to be doing lifecycle
> > management on strings inside this structure considering the only other
> > string I see is marked const. If you have a better way of doing this I'm
> > interested in hearing it but it doesn't seem reasonable to be storing
> > the xattr itself in the sysfs_dirent.
> 
> Smack depends on the xattr interfaces to inspect and manipulate labels
> on file system objects. Now you have a file system that "supports"
> xattrs, but not the xattr interfaces. What if I want to change the
> label on a sysfs entry? Or even read it? I can't with your scheme.
> 
> You are proposing a one-off hack to solve a particular problem. It
> introduces issues of its own. I don't care that it is clever and
> compact. It's not right.
> 

Why can't you use that? I used the set and get xattr interfaces to test
the code and it worked just fine. I agree with you that generic xattr
support for all pseudo file systems would be great but we don't see a
usecase for xattr support on sysfs other than in the security namespace.
Now we have a file system here that is not persistent across reboots and
we may only have one LSM active at a given time. You can try to have an
xattr table embedded into the sysfs_dirent structure but it is counter
the problem that is trying to be solved. The reason why we can't just
use the inode for the entry to begin with is that under memory pressure
those inodes can and will be evicted. Given that adding a method that
with the potential to add lots of memory pressure to the system seems
counterproductive. Remember those sysfs_dirent entries have to exist for
the lifetime of the sysfs filesystem. This means that you have the
potential for full page size attributes to be in each and every
sysfs_dirent structure.

The solution we have here says you specify and retrieve the security
blob using the normal xattr format but internally for memory reasons the
LSM is expected to map that blob to a unique identifier which is stored
in the sysfs_dirent. In the case of Smack this is practically a null
translation since your secid is the pointer to your actual label. In
SELinux the initial translation (luckily only done once) is a little
heavy weight but refreshing the inode after that is not. This mapping
can be transient because these xattrs are not persistent and neither is
the file system they are on.

Dave

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