From f6fbd8cbf3ed1915a7b957f2801f7c306a686c08 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 9 Nov 2022 14:14:35 -0500 Subject: 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 Reviewed-by: Mimi Zohar Signed-off-by: Paul Moore --- security/apparmor/domain.c | 3 +-- security/commoncap.c | 22 ++++++++++------------ security/integrity/evm/evm_crypto.c | 5 +++-- security/integrity/evm/evm_main.c | 7 +++++-- security/integrity/ima/ima.h | 5 +++-- security/integrity/ima/ima_appraise.c | 6 +++--- security/integrity/ima/ima_main.c | 6 ++++-- security/integrity/ima/ima_template_lib.c | 11 +++++------ 8 files changed, 34 insertions(+), 31 deletions(-) (limited to 'security') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 91689d34d281..04a818d51604 100644 --- 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 5fc8986c3c77..d4fc89095513 100644 --- 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 708de9656bbd..fa5ff13fa8c9 100644 --- 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 23d484e05e6f..bce72e80fd12 100644 --- 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 be965a8715e4..03b440921e61 100644 --- 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 3e0fbbd99534..88ffb15ca0e2 100644 --- 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 040b03ddc1c7..0226899947d6 100644 --- 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 7bf9b1507220..4564faae7d67 100644 --- 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; } -- cgit v1.2.3