2010-05-17 16:00:48

by Pavel Emelyanov

Subject: [PATCH] nfsd: Fix uninitialized ptr access in find_file()

The alloc_init_file() first adds a file to the hash and then
initializes its fi_inode, fi_id and fi_had_conflict.

The uninitialized fi_inode can thus be erroneously checked by
the find_file(), so move the hash insertion lower.

I didn't find whether the same can be true for two other fields,
but the common sense tells me it's better to initialize an object
before putting it into a global hash table :)

Signed-off-by: Pavel Emelyanov <[email protected]>

---

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 84b0fe9..296eded 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1757,12 +1757,12 @@ alloc_init_file(struct inode *ino)
INIT_LIST_HEAD(&fp->fi_hash);
INIT_LIST_HEAD(&fp->fi_stateids);
INIT_LIST_HEAD(&fp->fi_delegations);
- spin_lock(&recall_lock);
- list_add(&fp->fi_hash, &file_hashtbl[hashval]);
- spin_unlock(&recall_lock);
fp->fi_inode = igrab(ino);
fp->fi_id = current_fileid++;
fp->fi_had_conflict = false;
+ spin_lock(&recall_lock);
+ list_add(&fp->fi_hash, &file_hashtbl[hashval]);
+ spin_unlock(&recall_lock);
return fp;
}
return NULL;

2010-05-18 15:41:06

by J.Bruce Fields

Subject: Re: [PATCH] nfsd: Fix uninitialized ptr access in find_file()

On Mon, May 17, 2010 at 08:00:37PM +0400, Pavel Emelyanov wrote:
> The alloc_init_file() first adds a file to the hash and then
> initializes its fi_inode, fi_id and fi_had_conflict.
>
> The uninitialized fi_inode can thus be erroneously checked by
> the find_file(), so move the hash insertion lower.
>
> I didn't find whether the same can be true for two other fields,
> but the common sense tells me it's better to initialize an object
> before putting it into a global hash table :)

This code (and any code which looks up items in the file_hashtbl) is
currently all under the big state mutex; the spinlock is necessary only
to protect against some callback code which may remove entries without
holding the state lock.

Nevertheless, we hope to eventually pare down the use of the state lock,
so this looks like an obvious bug waiting to happen. Applied for 2.6.35
(with changelog edits to clarify the risk). Thanks!

--b.

>
> Signed-off-by: Pavel Emelyanov <[email protected]>
>
> ---
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 84b0fe9..296eded 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1757,12 +1757,12 @@ alloc_init_file(struct inode *ino)
> INIT_LIST_HEAD(&fp->fi_hash);
> INIT_LIST_HEAD(&fp->fi_stateids);
> INIT_LIST_HEAD(&fp->fi_delegations);
> - spin_lock(&recall_lock);
> - list_add(&fp->fi_hash, &file_hashtbl[hashval]);
> - spin_unlock(&recall_lock);
> fp->fi_inode = igrab(ino);
> fp->fi_id = current_fileid++;
> fp->fi_had_conflict = false;
> + spin_lock(&recall_lock);
> + list_add(&fp->fi_hash, &file_hashtbl[hashval]);
> + spin_unlock(&recall_lock);
> return fp;
> }
> return NULL;
>