linux-kernel - [PATCH][rfc] xattr acl: Suspicious use of potentially null pointer.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1101062247420.13988@swampdragon.chaosbits.net>
Date:	Thu, 6 Jan 2011 22:52:22 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	linux-fsdevel@...r.kernel.org
cc:	linux-kernel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>
Subject: [PATCH][rfc] xattr acl: Suspicious use of potentially null
 pointer.


In posix_acl_from_xattr() we have this at the head of the function:

  		posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
   		posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;

Since 'value' is passed in by the caller and may be NULL, the second line 
looks suspicious to me - taking a potentially NULL pointer (at least 
btrfs will pass something allocated with kmalloc() which may be NULL), 
adding one to it and casting it does not seem like it would always be such 
a hot idea.

How about rewriting it as per the patch below?


Signed-off-by: Jesper Juhl <jj@...osbits.net>
---
 xattr_acl.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/xattr_acl.c b/fs/xattr_acl.c
index 8d5a506..ff0a3e6 100644
--- a/fs/xattr_acl.c
+++ b/fs/xattr_acl.c
@@ -17,14 +17,18 @@
 struct posix_acl *
 posix_acl_from_xattr(const void *value, size_t size)
 {
-	posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
-	posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
+	posix_acl_xattr_header *header;
+	posix_acl_xattr_entry *entry, *end;
 	int count;
 	struct posix_acl *acl;
 	struct posix_acl_entry *acl_e;
 
 	if (!value)
 		return NULL;
+
+	header = (posix_acl_xattr_header *)value;
+	entry = (posix_acl_xattr_entry *)(header+1)
+
 	if (size < sizeof(posix_acl_xattr_header))
 		 return ERR_PTR(-EINVAL);
 	if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
@@ -35,12 +39,12 @@ posix_acl_from_xattr(const void *value, size_t size)
 		return ERR_PTR(-EINVAL);
 	if (count == 0)
 		return NULL;
-	
+
 	acl = posix_acl_alloc(count, GFP_NOFS);
 	if (!acl)
 		return ERR_PTR(-ENOMEM);
 	acl_e = acl->a_entries;
-	
+
 	for (end = entry + count; entry != end; acl_e++, entry++) {
 		acl_e->e_tag  = le16_to_cpu(entry->e_tag);
 		acl_e->e_perm = le16_to_cpu(entry->e_perm);



-- 
Jesper Juhl <jj@...osbits.net>            http://www.chaosbits.net/
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/