On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@xxxxxxxxxxxxx):
> > This patch modifies the interface to inode_getsecurity to have the
> > function return a buffer containing the security blob and its length via
> > parameters instead of relying on the calling function to give it an
> > appropriately sized buffer. Security blobs obtained with this function
> > should be freed using the release_secctx LSM hook. This alleviates the
> > problem of the caller having to guess a length and preallocate a buffer
> > for this function allowing it to be used elsewhere for Labeled NFS. The
> > patch also removed the unused err parameter. The conversion is similar
> > to the one performed by Al Viro for the security_getprocattr hook.
> >
> > Signed-off-by: David P. Quigley <dpquigl@xxxxxxxxxxxxx>
> > ---
> > fs/xattr.c | 26 ++++++++++++++++++++++++--
> > include/linux/security.h | 27 ++++++++++++++-------------
> > include/linux/xattr.h | 1 +
> > mm/shmem.c | 3 +--
> > security/dummy.c | 4 +++-
> > security/selinux/hooks.c | 38 ++++++++++----------------------------
>
> (Hmm, I was about to ask if this diffstat could be complete, as it
> doesn't have for instance security/security.c, but I guess this predates
> the staticlsm patch...)
It wouldn't be much effort to rebase this patch against Linus's latest
tree. I am assuming that the static lsm patch is in there based on the
recent discussion on LKML?
>
> > 6 files changed, 53 insertions(+), 46 deletions(-)
> >
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index a44fd92..d45c7ef 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -105,6 +105,29 @@ out:
> > EXPORT_SYMBOL_GPL(vfs_setxattr);
> >
> > ssize_t
> > +xattr_getsecurity(struct inode *inode, const char *name, void *value,
> > + size_t size)
> > +{
> > + void *buffer = NULL;
> > + ssize_t len;
> > +
> > + len = security_inode_getsecurity(inode, name, &buffer);
> > + if (len < 0)
> > + return len;
> > + if (!value || !size)
> > + goto out;
> > + if (size < len) {
> > + len = -ERANGE;
> > + goto out;
> > + }
> > + memcpy(value, buffer, len);
> > +out:
> > + security_release_secctx(buffer, len);
>
> This is mighty misleading in -ERANGE case :) I realize that
> selinux_release_secctx() ignores len anyway. But given the description
> in security.h, I'd say either you need to keep the actual length
> allocated and pass that in here, or (probably better) have another patch
> remove the second argument from security_release_secctx().
I would consider this a bug on my part. I don't think we can get rid of
the len argument to security_release_secctx because it is supposed to be
a generic destructor for security blobs. If the module always returned a
fixed length blob we could remove it but there is no telling what one of
the many new LSMs to come will do.
>
> > + return len;
> > +}
> > +EXPORT_SYMBOL_GPL(xattr_getsecurity);
> > +
> > +ssize_t
> > vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
> > {
> > struct inode *inode = dentry->d_inode;
> > @@ -126,8 +149,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
> > if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > XATTR_SECURITY_PREFIX_LEN)) {
> > const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > - int ret = security_inode_getsecurity(inode, suffix, value,
> > - size, error);
> > + int ret = xattr_getsecurity(inode, suffix, value, size);
> > /*
> > * Only overwrite the return value if a security module
> > * is actually active.
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 1a15526..8658929 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -391,15 +391,11 @@ struct request_sock;
> > * identified by @name for @dentry.
> > * Return 0 if permission is granted.
> > * @inode_getsecurity:
> > - * Copy the extended attribute representation of the security label
> > - * associated with @name for @inode into @buffer. @buffer may be
> > - * NULL to request the size of the buffer required. @size indicates
> > - * the size of @buffer in bytes. Note that @name is the remainder
> > - * of the attribute name after the security. prefix has been removed.
> > - * @err is the return value from the preceding fs getxattr call,
> > - * and can be used by the security module to determine whether it
> > - * should try and canonicalize the attribute value.
> > - * Return number of bytes used/required on success.
> > + * Retrieve a copy of the extended attribute representation of the
> > + * security label associated with @name for @inode via @buffer. Note that
> > + * @name is the remainder of the attribute name after the security prefix
> > + * has been removed.
> > + * Return size of buffer on success.
> > * @inode_setsecurity:
> > * Set the security label associated with @name for @inode from the
> > * extended attribute value @value. @size indicates the size of the
> > @@ -1233,7 +1229,8 @@ struct security_operations {
> > int (*inode_listxattr) (struct dentry *dentry);
> > int (*inode_removexattr) (struct dentry *dentry, char *name);
> > const char *(*inode_xattr_getsuffix) (void);
> > - int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
> > + int (*inode_getsecurity)(const struct inode *inode, const char *name,
> > + void **buffer);
> > int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
> > int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
> >
> > @@ -1777,11 +1774,13 @@ static inline const char *security_inode_xattr_getsuffix(void)
> > return security_ops->inode_xattr_getsuffix();
> > }
> >
> > -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static inline int security_inode_getsecurity(const struct inode *inode,
> > + const char *name,
> > + void **buffer)
> > {
> > if (unlikely (IS_PRIVATE (inode)))
> > return 0;
> > - return security_ops->inode_getsecurity(inode, name, buffer, size, err);
> > + return security_ops->inode_getsecurity(inode, name, buffer);
> > }
> >
> > static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> > @@ -2468,7 +2467,9 @@ static inline const char *security_inode_xattr_getsuffix (void)
> > return NULL ;
> > }
> >
> > -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static inline int security_inode_getsecurity(const struct inode *inode,
> > + const char *name,
> > + void **buffer)
> > {
> > return -EOPNOTSUPP;
> > }
> > diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> > index def131a..df6b95d 100644
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -46,6 +46,7 @@ struct xattr_handler {
> > size_t size, int flags);
> > };
> >
> > +ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
> > ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
> > ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> > int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index fcd19d3..5ed3a66 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1958,8 +1958,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
> > {
> > if (strcmp(name, "") == 0)
> > return -EINVAL;
> > - return security_inode_getsecurity(inode, name, buffer, size,
> > - -EOPNOTSUPP);
> > + return xattr_getsecurity(inode, name, buffer, size);
> > }
> >
> > static int shmem_xattr_security_set(struct inode *inode, const char *name,
> > diff --git a/security/dummy.c b/security/dummy.c
> > index 853ec22..6983db9 100644
> > --- a/security/dummy.c
> > +++ b/security/dummy.c
> > @@ -377,7 +377,9 @@ static int dummy_inode_removexattr (struct dentry *dentry, char *name)
> > return 0;
> > }
> >
> > -static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static int dummy_inode_getsecurity(const struct inode *inode,
> > + const char *name,
> > + void **buffer)
> > {
> > return -EOPNOTSUPP;
> > }
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0753b20..2756c99 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -125,32 +125,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
> >
> > static struct kmem_cache *sel_inode_cache;
> >
> > -/* Return security context for a given sid or just the context
> > - length if the buffer is null or length is 0 */
> > -static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> > -{
> > - char *context;
> > - unsigned len;
> > - int rc;
> > -
> > - rc = security_sid_to_context(sid, &context, &len);
> > - if (rc)
> > - return rc;
> > -
> > - if (!buffer || !size)
> > - goto getsecurity_exit;
> > -
> > - if (size < len) {
> > - len = -ERANGE;
> > - goto getsecurity_exit;
> > - }
> > - memcpy(buffer, context, len);
> > -
> > -getsecurity_exit:
> > - kfree(context);
> > - return len;
> > -}
> > -
> > /* Allocate and free functions for each kind of security blob. */
> >
> > static int task_alloc_security(struct task_struct *task)
> > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > *
> > * Permission check is handled by selinux_inode_getxattr hook.
> > */
> > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static int selinux_inode_getsecurity(const struct inode *inode,
> > + const char *name,
> > + void **buffer)
> > {
> > + u32 size;
> > + int error;
> > struct inode_security_struct *isec = inode->i_security;
> >
> > if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > return -EOPNOTSUPP;
> >
> > - return selinux_getsecurity(isec->sid, buffer, size);
> > + error = security_sid_to_context(isec->sid, (char **)buffer, &size);
>
> The only other downside I see here is that when the user just passes in
> NULL for a buffer, security_sid_to_context() will still
> kmalloc the buffer only to have it immediately freed by
> xattr_getsecurity() through release_secctx(). I trust that isn't seen
> as any major performance impact?
There is no way to avoid this in the SELinux case. SELinux doesn't store
the sid to string mapping directly. Rather it takes the sid and then
builds the string from fields in the related structure. So regardless
this data is being allocated internally. The only issue I potentially
see is that if someone passes in null expecting just to get the length
we are actually returning a value. However we are changing the semantics
of the function so the old semantics are no longer valid.
In the case of SMACK this is equally irrelevant since there is no
allocation at all just the returning of a pointer into a table via
buffer. I don't know enough about other potential LSMs to comment on the
impact on their code bases.
>
> thanks,
> -serge
>
> > + if (error)
> > + return error;
> > + error = size;
> > + return error;
> > }
> >
> > static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> -
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
-
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
