lsm,fs: fix vfs_getxattr_alloc() return type and caller error paths

The vfs_getxattr_alloc() function currently returns a ssize_t value
despite the fact that it only uses int values internally for return
values.  Fix this by converting vfs_getxattr_alloc() to return an
int type and adjust the callers as necessary.  As part of these
caller modifications, some of the callers are fixed to properly free
the xattr value buffer on both success and failure to ensure that
memory is not leaked in the failure case.

Reviewed-by: Serge Hallyn <serge@hallyn.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>

diff --git a/fs/xattr.c b/fs/xattr.c
index 61107b6bbed29211786d2eda0318499d95f525fe..f38fd969b5fcd177f8e84b270f74cfefb04c687b 100644 (file)
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -354,11 +354,12 @@ out_noalloc:
  * vfs_getxattr_alloc - allocate memory, if necessary, before calling getxattr
  *
  * Allocate memory, if not already allocated, or re-allocate correct size,
- * before retrieving the extended attribute.
+ * before retrieving the extended attribute.  The xattr value buffer should
+ * always be freed by the caller, even on error.
  *
  * Returns the result of alloc, if failed, or the getxattr operation.
  */
-ssize_t
+int
 vfs_getxattr_alloc(struct user_namespace *mnt_userns, struct dentry *dentry,
                   const char *name, char **xattr_value, size_t xattr_size,
                   gfp_t flags)

diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 4c379d23ec6e73b9590fba7c16e1152fb79376fd..2218a9645b89d6daa66982c62c917b0f895ec6b4 100644 (file)
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -68,9 +68,9 @@ int __vfs_removexattr_locked(struct user_namespace *, struct dentry *,
 int vfs_removexattr(struct user_namespace *, struct dentry *, const char *);
 
 ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size);
-ssize_t vfs_getxattr_alloc(struct user_namespace *mnt_userns,
-                          struct dentry *dentry, const char *name,
-                          char **xattr_value, size_t size, gfp_t flags);
+int vfs_getxattr_alloc(struct user_namespace *mnt_userns,
+                      struct dentry *dentry, const char *name,
+                      char **xattr_value, size_t size, gfp_t flags);
 
 int xattr_supported_namespace(struct inode *inode, const char *prefix);
 

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 91689d34d2811db3fef09381248f1c91ae361439..04a818d5160475d1bfb73a4faef1aeafd45afda4 100644 (file)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -311,10 +311,9 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
                           struct aa_profile *profile, unsigned int state)
 {
        int i;
-       ssize_t size;
        struct dentry *d;
        char *value = NULL;
-       int value_size = 0, ret = profile->xattr_count;
+       int size, value_size = 0, ret = profile->xattr_count;
 
        if (!bprm || !profile->xattr_count)
                return 0;

diff --git a/security/commoncap.c b/security/commoncap.c
index 5fc8986c3c77cd1dccfada0b8b35597de5f666aa..d4fc8909551346b1c2fdd60f165aaddec9a70ef0 100644 (file)
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -350,14 +350,14 @@ static __u32 sansflags(__u32 m)
        return m & ~VFS_CAP_FLAGS_EFFECTIVE;
 }
 
-static bool is_v2header(size_t size, const struct vfs_cap_data *cap)
+static bool is_v2header(int size, const struct vfs_cap_data *cap)
 {
        if (size != XATTR_CAPS_SZ_2)
                return false;
        return sansflags(le32_to_cpu(cap->magic_etc)) == VFS_CAP_REVISION_2;
 }
 
-static bool is_v3header(size_t size, const struct vfs_cap_data *cap)
+static bool is_v3header(int size, const struct vfs_cap_data *cap)
 {
        if (size != XATTR_CAPS_SZ_3)
                return false;
@@ -379,7 +379,7 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns,
                          struct inode *inode, const char *name, void **buffer,
                          bool alloc)
 {
-       int size, ret;
+       int size;
        kuid_t kroot;
        u32 nsmagic, magic;
        uid_t root, mappedroot;
@@ -395,20 +395,18 @@ int cap_inode_getsecurity(struct user_namespace *mnt_userns,
        dentry = d_find_any_alias(inode);
        if (!dentry)
                return -EINVAL;
-
-       size = sizeof(struct vfs_ns_cap_data);
-       ret = (int)vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS,
-                                     &tmpbuf, size, GFP_NOFS);
+       size = vfs_getxattr_alloc(mnt_userns, dentry, XATTR_NAME_CAPS, &tmpbuf,
+                                 sizeof(struct vfs_ns_cap_data), GFP_NOFS);
        dput(dentry);
-
-       if (ret < 0 || !tmpbuf)
-               return ret;
+       /* gcc11 complains if we don't check for !tmpbuf */
+       if (size < 0 || !tmpbuf)
+               goto out_free;
 
        fs_ns = inode->i_sb->s_user_ns;
        cap = (struct vfs_cap_data *) tmpbuf;
-       if (is_v2header((size_t) ret, cap)) {
+       if (is_v2header(size, cap)) {
                root = 0;
-       } else if (is_v3header((size_t) ret, cap)) {
+       } else if (is_v3header(size, cap)) {
                nscap = (struct vfs_ns_cap_data *) tmpbuf;
                root = le32_to_cpu(nscap->rootid);
        } else {

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 708de9656bbd2a5b4c106e577abdecea8dfff022..fa5ff13fa8c976b8db5034cd079fcd4ce4700719 100644 (file)
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -335,14 +335,15 @@ static int evm_is_immutable(struct dentry *dentry, struct inode *inode)
                                (char **)&xattr_data, 0, GFP_NOFS);
        if (rc <= 0) {
                if (rc == -ENODATA)
-                       return 0;
-               return rc;
+                       rc = 0;
+               goto out;
        }
        if (xattr_data->type == EVM_XATTR_PORTABLE_DIGSIG)
                rc = 1;
        else
                rc = 0;
 
+out:
        kfree(xattr_data);
        return rc;
 }

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 23d484e05e6f2f59975e0cbdd4e1ea9c479142cd..bce72e80fd1237ab48ebae0b66bbd76fd96f3714 100644 (file)
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -519,14 +519,17 @@ static int evm_xattr_change(struct user_namespace *mnt_userns,
 
        rc = vfs_getxattr_alloc(&init_user_ns, dentry, xattr_name, &xattr_data,
                                0, GFP_NOFS);
-       if (rc < 0)
-               return 1;
+       if (rc < 0) {
+               rc = 1;
+               goto out;
+       }
 
        if (rc == xattr_value_len)
                rc = !!memcmp(xattr_value, xattr_data, rc);
        else
                rc = 1;
 
+out:
        kfree(xattr_data);
        return rc;
 }

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index be965a8715e4e2404120b408ab168fc17a81a5a0..03b440921e61505f0829fad1b600a4c15578610b 100644 (file)
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -326,7 +326,7 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
 enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
                                 int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
-                  struct evm_ima_xattr_data **xattr_value);
+                  struct evm_ima_xattr_data **xattr_value, int xattr_len);
 
 #else
 static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
@@ -372,7 +372,8 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
 }
 
 static inline int ima_read_xattr(struct dentry *dentry,
-                                struct evm_ima_xattr_data **xattr_value)
+                                struct evm_ima_xattr_data **xattr_value,
+                                int xattr_len)
 {
        return 0;
 }

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 3e0fbbd995342fcf80be6d5254bfe1bd78e01edc..88ffb15ca0e2ee070b01527dadaeef7c82b1b5d9 100644 (file)
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -221,12 +221,12 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
 }
 
 int ima_read_xattr(struct dentry *dentry,
-                  struct evm_ima_xattr_data **xattr_value)
+                  struct evm_ima_xattr_data **xattr_value, int xattr_len)
 {
-       ssize_t ret;
+       int ret;
 
        ret = vfs_getxattr_alloc(&init_user_ns, dentry, XATTR_NAME_IMA,
-                                (char **)xattr_value, 0, GFP_NOFS);
+                                (char **)xattr_value, xattr_len, GFP_NOFS);
        if (ret == -EOPNOTSUPP)
                ret = 0;
        return ret;

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 040b03ddc1c776665c75fde3081a7d9775d17041..0226899947d6a54b61a18a6ba178f8aecfb6c8b7 100644 (file)
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -293,7 +293,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
        /* HASH sets the digital signature and update flags, nothing else */
        if ((action & IMA_HASH) &&
            !(test_bit(IMA_DIGSIG, &iint->atomic_flags))) {
-               xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+               xattr_len = ima_read_xattr(file_dentry(file),
+                                          &xattr_value, xattr_len);
                if ((xattr_value && xattr_len > 2) &&
                    (xattr_value->type == EVM_IMA_XATTR_DIGSIG))
                        set_bit(IMA_DIGSIG, &iint->atomic_flags);
@@ -316,7 +317,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
        if ((action & IMA_APPRAISE_SUBMASK) ||
            strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
                /* read 'security.ima' */
-               xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+               xattr_len = ima_read_xattr(file_dentry(file),
+                                          &xattr_value, xattr_len);
 
                /*
                 * Read the appended modsig if allowed by the policy, and allow

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 7bf9b1507220239e75856922a9a0d0d686e67f53..4564faae7d673762db342f315f342563646cd093 100644 (file)
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -601,16 +601,15 @@ int ima_eventevmsig_init(struct ima_event_data *event_data,
        rc = vfs_getxattr_alloc(&init_user_ns, file_dentry(event_data->file),
                                XATTR_NAME_EVM, (char **)&xattr_data, 0,
                                GFP_NOFS);
-       if (rc <= 0)
-               return 0;
-
-       if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
-               kfree(xattr_data);
-               return 0;
+       if (rc <= 0 || xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
+               rc = 0;
+               goto out;
        }
 
        rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
                                           field_data);
+
+out:
        kfree(xattr_data);
        return rc;
 }