linux-kernel - [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

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: <200610272316.47089.jesper.juhl@gmail.com>
Date:	Fri, 27 Oct 2006 23:16:46 +0200
From:	Jesper Juhl <jesper.juhl@...il.com>
To:	linux-kernel@...r.kernel.org
Cc:	Andreas Gruenbacher <agruen@...e.de>,
	Neil Brown <neilb@....unsw.edu.au>, nfs@...ts.sourceforge.net,
	Andrew Morton <akpm@...l.org>,
	Jesper Juhl <jesper.juhl@...il.com>
Subject: [PATCH] NFS: nfsaclsvc_encode_getaclres() - Fix potential NULL deref and tiny optimization.

In fs/nfsd/nfs2acl.c::nfsaclsvc_encode_getaclres() I see a few issues.

1) At the top of the function we assign to the 'inode' variable by 
dereferencing 'dentry', but further down we test 'dentry' for NULL. So, if 
'dentry' (which is really 'resp->fh.fh_dentry') can be NULL, then either 
we have a potential NULL pointer deref bug or we have a superflous test.

2) In the case of  resp->fh.fh_dentry  != NULL  we have a duplicate 
assignment to 'inode' - just one would do.

3) There are two locations in the function where we may return before we 
use the value of the variable 'w', but we compute it at the very top of the 
function. So in the case where we return early we have wasted a few cycles 
computing a value that was never used.

This patch solves these issues by :

1) Moving the computation of 'w' to just before it is used, after the two 
potential returns.

2) Remove the initial assignment of 'dentry->d_inode' 
(aka resp->fh.fh_dentry->d_inode) to 'inode' so that the assignment only 
happens once and happens after the NULL test.

So we get 3 benefits from this patch : 

1) We avoid a potential NULL pointer deref.

2) We save a few CPU cycles from not computing 'w' in the case of an early 
return as well as saving the assignment to 'inode' in the  dentry == NULL 
case, and there's only a single assignment to 'inode' ever.

3) We save a few bytes of .text in the object file.
    before:
        text    data     bss     dec     hex filename
        2502     204       0    2706     a92 fs/nfsd/nfs2acl.o
    after:
        text    data     bss     dec     hex filename
        2489     204       0    2693     a85 fs/nfsd/nfs2acl.o

Patch is compile tested only since I don't currently have a setup where I 
can meaningfully test it further.

Please comment and/or apply.

Signed-off-by: Jesper Juhl <jesper.juhl@...il.com>
---

 fs/nfsd/nfs2acl.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs2acl.c b/fs/nfsd/nfs2acl.c
index e3eca08..d89d63f 100644
--- a/fs/nfsd/nfs2acl.c
+++ b/fs/nfsd/nfs2acl.c
@@ -221,12 +221,10 @@ static int nfsaclsvc_encode_getaclres(st
 		struct nfsd3_getaclres *resp)
 {
 	struct dentry *dentry = resp->fh.fh_dentry;
-	struct inode *inode = dentry->d_inode;
-	int w = nfsacl_size(
-		(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
-		(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 	struct kvec *head = rqstp->rq_res.head;
+	struct inode *inode;
 	unsigned int base;
+	int w;
 	int n;

 	if (dentry == NULL || dentry->d_inode == NULL)
@@ -239,6 +237,9 @@ static int nfsaclsvc_encode_getaclres(st
 		return 0;
 	base = (char *)p - (char *)head->iov_base;

+	w = nfsacl_size(
+		(resp->mask & NFS_ACL)   ? resp->acl_access  : NULL,
+		(resp->mask & NFS_DFACL) ? resp->acl_default : NULL);
 	rqstp->rq_res.page_len = w;
 	while (w > 0) {
 		if (!rqstp->rq_respages[rqstp->rq_resused++])

-- 
Jesper Juhl <jesper.juhl@...il.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

-
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/