LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
@ 2018-05-27 22:55 Colin King
2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:30 ` Mimi Zohar
0 siblings, 2 replies; 4+ messages in thread
From: Colin King @ 2018-05-27 22:55 UTC (permalink / raw)
To: Matthew Garrett, Mimi Zohar, James Morris, Serge E . Hallyn,
linux-integrity, linux-security-module
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
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 <colin.king@canonical.com>
---
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
@ 2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:31 ` Mimi Zohar
2018-05-29 12:30 ` Mimi Zohar
1 sibling, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2018-05-29 9:05 UTC (permalink / raw)
To: Colin King
Cc: Matthew Garrett, Mimi Zohar, James Morris, Serge E . Hallyn,
linux-integrity, linux-security-module, kernel-janitors,
linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
2018-05-29 9:05 ` Dan Carpenter
@ 2018-05-29 12:30 ` Mimi Zohar
1 sibling, 0 replies; 4+ messages in thread
From: Mimi Zohar @ 2018-05-29 12:30 UTC (permalink / raw)
To: Colin King, Matthew Garrett, James Morris, Serge E . Hallyn,
linux-integrity, linux-security-module
Cc: kernel-janitors, linux-kernel
Hi Colin,
On Sun, 2018-05-27 at 23:55 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> 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 <colin.king@canonical.com>
> ---
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate
2018-05-29 9:05 ` Dan Carpenter
@ 2018-05-29 12:31 ` Mimi Zohar
0 siblings, 0 replies; 4+ messages in thread
From: Mimi Zohar @ 2018-05-29 12:31 UTC (permalink / raw)
To: Dan Carpenter, Colin King
Cc: Matthew Garrett, James Morris, Serge E . Hallyn, linux-integrity,
linux-security-module, kernel-janitors, linux-kernel
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-29 12:31 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-27 22:55 [PATCH] EVM: Fix null dereference on xattr when xattr fails to allocate Colin King
2018-05-29 9:05 ` Dan Carpenter
2018-05-29 12:31 ` Mimi Zohar
2018-05-29 12:30 ` Mimi Zohar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).