2018-05-27 22:55:39

by Colin King

Subject: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate

From: Colin Ian King <[email protected]>

In the case where the allocation of xattr fails and xattr is NULL, the
error exit return path via label 'out' will dereference xattr when
kfree'ing xattr-name. Fix this by only kfree'ing xattr->name and xattr
when xattr is non-null.

Detected by CoverityScan, CID#1469366 ("Dereference after null check")

Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
Signed-off-by: Colin Ian King <[email protected]>
---
security/integrity/evm/evm_secfs.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index fb8bc950aceb..cf5cd303d7c0 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -253,8 +253,10 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
out:
audit_log_format(ab, " res=%d", err);
audit_log_end(ab);
- kfree(xattr->name);
- kfree(xattr);
+ if (xattr) {
+ kfree(xattr->name);
+ kfree(xattr);
+ }
return err;
}

--
2.17.0

2018-05-29 09:06:48

by Dan Carpenter

Subject: Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate

Not really related to this patch except I was looking at the function:

security/integrity/evm/evm_secfs.c
191 ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
192 if (IS_ERR(ab))
193 return PTR_ERR(ab);
194
195 xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
196 if (!xattr) {
197 err = -ENOMEM;
198 goto out;
199 }
200
201 xattr->name = memdup_user_nul(buf, count);
202 if (IS_ERR(xattr->name)) {
203 err = PTR_ERR(xattr->name);
204 xattr->name = NULL;
205 goto out;
206 }
207
208 /* Remove any trailing newline */
209 len = strlen(xattr->name);
210 if (xattr->name[len-1] == '\n')

strlen() could be zero, leading to a read underflow here.

211 xattr->name[len-1] = '\0';
212
213 if (strcmp(xattr->name, ".") == 0) {
214 evm_xattrs_locked = 1;
215 newattrs.ia_mode = S_IFREG | 0440;
216 newattrs.ia_valid = ATTR_MODE;
217 inode = evm_xattrs->d_inode;
218 inode_lock(inode);
219 err = simple_setattr(evm_xattrs, &newattrs);
220 inode_unlock(inode);
221 audit_log_format(ab, "locked");
222 if (!err)
223 err = count;
224 goto out;
225 }

regards,
dan carpenter

2018-05-29 12:31:54

by Mimi Zohar

Subject: Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate

Hi Colin,

On Sun, 2018-05-27 at 23:55 +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> In the case where the allocation of xattr fails and xattr is NULL, the
> error exit return path via label 'out' will dereference xattr when
> kfree'ing xattr-name. Fix this by only kfree'ing xattr->name and xattr
> when xattr is non-null.
>
> Detected by CoverityScan, CID#1469366 ("Dereference after null check")
>
> Fixes: fa516b66a1bf ("EVM: Allow runtime modification of the set of verified xattrs")
> Signed-off-by: Colin Ian King <[email protected]>
> ---
> security/integrity/evm/evm_secfs.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index fb8bc950aceb..cf5cd303d7c0 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -253,8 +253,10 @@ static ssize_t evm_write_xattrs(struct file *file, const char __user *buf,
> out:
> audit_log_format(ab, " res=%d", err);
> audit_log_end(ab);
> - kfree(xattr->name);
> - kfree(xattr);
> + if (xattr) {
> + kfree(xattr->name);
> + kfree(xattr);
> + }
> return err;
> }
>

Thanks!  To fix this problem, I think more is needed.

Without the xattr, there is nothing to audit except the attempt to
extend the xattr list.  Failure to allocate the xattr or xattr->name
should either result in a different audit message or return
immediately without any audit message.

Mimi

2018-05-29 12:34:27

by Mimi Zohar

Subject: Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate

Hi Dan,

On Tue, 2018-05-29 at 12:05 +0300, Dan Carpenter wrote:
> Not really related to this patch except I was looking at the function:
>
> security/integrity/evm/evm_secfs.c
> 191 ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_EVM_XATTR);
> 192 if (IS_ERR(ab))
> 193 return PTR_ERR(ab);
> 194
> 195 xattr = kmalloc(sizeof(struct xattr_list), GFP_KERNEL);
> 196 if (!xattr) {
> 197 err = -ENOMEM;
> 198 goto out;
> 199 }
> 200
> 201 xattr->name = memdup_user_nul(buf, count);
> 202 if (IS_ERR(xattr->name)) {
> 203 err = PTR_ERR(xattr->name);
> 204 xattr->name = NULL;
> 205 goto out;
> 206 }
> 207
> 208 /* Remove any trailing newline */
> 209 len = strlen(xattr->name);
> 210 if (xattr->name[len-1] == '\n')
>
> strlen() could be zero, leading to a read underflow here.

Thanks!  Could you modify the maximum xattr size check (before this
code snippet) to check for underflow?

Mimi


>
> 211 xattr->name[len-1] = '\0';
> 212
> 213 if (strcmp(xattr->name, ".") == 0) {
> 214 evm_xattrs_locked = 1;
> 215 newattrs.ia_mode = S_IFREG | 0440;
> 216 newattrs.ia_valid = ATTR_MODE;
> 217 inode = evm_xattrs->d_inode;
> 218 inode_lock(inode);
> 219 err = simple_setattr(evm_xattrs, &newattrs);
> 220 inode_unlock(inode);
> 221 audit_log_format(ab, "locked");
> 222 if (!err)
> 223 err = count;
> 224 goto out;
> 225 }
>
> regards,
> dan carpenter
>