Stephen Smalley wrote:
> On Fri, 2009-08-14 at 08:20 -0400, Stephen Smalley wrote:
>
>> ...
>>> + */
>>> +static DEFINE_MUTEX(sysfs_xattr_lock);
>>> +
>>> +static struct sysfs_xattr *new_xattr(const char *name, const void *value,
>>> + size_t size)
>>> +{
>>> + struct sysfs_xattr *nxattr;
>>> + void *nvalue;
>>> + char *nname;
>>> +
>>> + nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
>>> + if (!nxattr)
>>> + return NULL;
>>> + nvalue = kzalloc(size, GFP_KERNEL);
>>> + if (!nvalue) {
>>> + kfree(nxattr);
>>> + return NULL;
>>> + }
>>> + nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
>>> + if (!nname) {
>>> + kfree(nxattr);
>>> + kfree(nvalue);
>>> + return NULL;
>>> + }
>>> + memcpy(nvalue, value, size);
>>> + strcpy(nname, name);
>>> + nxattr->sx_name = nname;
>>> + nxattr->sx_value = nvalue;
>>> + nxattr->sx_size = size;
>>>
>> Storing the name/value pairs here is redundant - the security module
>> already has to store the value in some form (potentially smaller, like a
>> secid + struct in the SELinux case). This wastes memory.
>>
>
> Sorry - to clarify, I understand that we have to store a representation
> of the security attribute in the backing data structure so that it can
> be restored later, but that representation should come from the security
> module rather than being the original (name, value, size) triple. Which
> is what David's patch does - he obtains a secid from the security module
> for storage in the wrapped iattr structure.
>
Sorry, but I disagree with your assertion. An LSM can do what
it likes with the xattr, but the value sent from userland is
what should be stored.
>>> +
>>> + return nxattr;
>>> +}
>>> +
>>> +int sysfs_setxattr(struct dentry *dentry, const char *name,
>>> + const void *value, size_t size, int flags)
>>> +{
>>> + struct sysfs_dirent *sd = dentry->d_fsdata;
>>> + struct list_head *xlist;
>>> + struct sysfs_xattr *nxattr;
>>> + void *nvalue;
>>> + int rc = 0;
>>> +
>>> + /*
>>> + * Only support the security namespace.
>>> + * Only allow privileged processes to set them.
>>> + * It has to be OK with the LSM, if any, as well.
>>> + */
>>> + if (strncmp(name, XATTR_SECURITY_PREFIX,
>>> + sizeof XATTR_SECURITY_PREFIX - 1))
>>> + return -ENOTSUPP;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>>
>> SELinux does not require CAP_SYS_ADMIN to set its attributes, so this
>> breaks its security model.
>>
>
> And you don't need to apply any permission check here, as it gets
> covered by the security_inode_setxattr() hook in vfs_setxattr() prior to
> invoking i_op->setxattr.
>
David seemed to think it necessary in an earlier review. I will
have another look.
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
