Message-ID: <4A8C91D4.3070308@schaufler-ca.com>
Date:	Wed, 19 Aug 2009 16:59:16 -0700
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Stephen Smalley <sds@...ho.nsa.gov>
CC:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	"David P. Quigley" <dpquigl@...ho.nsa.gov>, jmorris@...ei.org,
	gregkh@...e.de, linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org, selinux@...ho.nsa.gov,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH] Security/sysfs: v2 - Enable security xattrs to be set
 on sysfs files, directories, and symlinks.

Stephen Smalley wrote:
> On Tue, 2009-08-18 at 21:37 -0700, Casey Schaufler wrote:
>   
>> Stephen Smalley wrote:
>>     
>>> On Tue, 2009-08-18 at 07:12 -0700, Casey Schaufler wrote:
>>>   
>>>       
>>>> Stephen Smalley wrote:
>>>>     
>>>>         
>>>>> On Mon, 2009-08-17 at 20:55 -0700, Casey Schaufler wrote:
>>>>>   
>>>>>       
>>>>>           
>>>>>> From: Casey Schaufler <casey@...aufler-ca.com>
>>>>>>
>>>>>> Another approach to limited xattr support in sysfs.
>>>>>>
>>>>>> I tried to listen to the objections to a linked list representation
>>>>>> and I think that I understand that there isn't really any interest
>>>>>> in supporting xattrs for real, only for those maintained by LSMs.
>>>>>> I also looked carefully into the claims that memory usage is
>>>>>> critical and that the code I had before was duplicating effort.
>>>>>>
>>>>>> This version lets the surrounding code do as much of the work as
>>>>>> possible. Unlike the initial proposal for sysfs xattrs, it does not
>>>>>> introduce any new LSM hooks, it uses hooks that already exist. It
>>>>>> does not support any attributes on its own, it only provides for
>>>>>> the attribute advertised by security_inode_listsecurity(). It could
>>>>>> easily be used by other filesystems to provide the same LSM xattr
>>>>>> support. It could also be extended to do the list based support for
>>>>>> arbitrary xattrs without too much effort.
>>>>>>
>>>>>> Probably the oddest bit is that the inode_getsecurity hooks need to
>>>>>> check to see if they are getting called before the inode is instantiated
>>>>>> and return -ENODATA in that event. It would be possible to do a
>>>>>> filesystem specific check instead, but this way provides for generally
>>>>>> correct behavior at small cost.
>>>>>>
>>>>>> This has been tested with Smack, but not SELinux. I think that
>>>>>> SELinux will work correctly, but it could be that a labeling
>>>>>> behavior that is different than the "usual" instantiation labeling
>>>>>> is actually desired. That would be an easy change.
>>>>>>
>>>>>> As always, let me know if I missed something obvious or if there's a
>>>>>> fatal flaw in the scheme.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> The point of the David's patch was to provide a way to save the security
>>>>> xattr in the backing data structure for sysfs entries when an attribute
>>>>> value is set from userspace so that the value can be preserved if the
>>>>> inode is evicted from memory and later re-instantiated.  AFAICS, your
>>>>> patch completely misses the problem.  How about we just go back to
>>>>> David's patch?
>>>>>   
>>>>>       
>>>>>           
>>>> Oh no, that would use too much memory!
>>>>
>>>> Either you care about the value the user set, in which case you
>>>> want to save the value the user set, or you don't. If you do, you
>>>> have to save that value, not an LSM's interpretation of that value.
>>>> No secids. No new hooks.
>>>>     
>>>>         
>>> As the security module is the only component of the kernel that
>>> uses/interprets that value, it isn't unreasonable for the security
>>> module's interpretation of that value to be considered canonical.  In
>>> fact, that is already the case - the hooks within vfs_getxattr() enable
>>> the security module to override/replace the actual security xattr value
>>> returned to userspace.
>>>
>>> In the case of Smack, Smack could just provide a pointer to its own
>>> internal copy of the string, and that could be stored in the wrapped
>>> iattr.  In the case of SELinux, we could provide a secid that could be
>>> stored in the wrapped iattr.  The hook interface could just handle it as
>>> a blob if you prefer.  But either way we don't need extra storage aside
>>> from a pointer-size field in the wrapped iattr.
>>>   
>>>       
>> So how often is the SELinux label going to get explicitly set in /sys ?
>> I'm grappling with the value of going hog-wild in optimizing this. If
>> it is something that's quite rare I can see the concern with expanding
>> the d_entry. If it is common, the storage associated with storing the
>> xattr could be an issue. If it is uncommon but not rare there's another
>> story again.
>>
>> I'm looking at addressing the issues. Thank you.
>>     
>
> I'd expect most sysfs nodes to be left in the default label, although we
> don't really know as this would be the first time that people have the
> option of finer-grained control to sysfs.
>
>   

So I still don't like the way it exposes LSM internal data to the
file system code, but given how long it's taking for me to create
a better solution I don't think that I can in all fairness say NAK
to David Quigley's sysfs patch any longer. I withdraw my objection,
while maintaining my reservations.

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

Powered by blists - more mailing lists