nfsd: memory corruption in nfsd4_lock()
diff mbox series

Message ID db0980d0-8c99-940a-1748-04e679a366d1@virtuozzo.com
State New
Headers show
Series
  • nfsd: memory corruption in nfsd4_lock()
Related show

Commit Message

Vasily Averin March 23, 2020, 7:55 a.m. UTC
New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
does not initialised nbl_list and nbl_lru.
If conflock allocation fails rollback can call list_del_init()
access uninitialized fields and corrupt memory.

Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
 fs/nfsd/nfs4state.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

Comments

Jeff Layton March 23, 2020, 12:18 p.m. UTC | #1
On Mon, 2020-03-23 at 10:55 +0300, Vasily Averin wrote:
> New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
> does not initialised nbl_list and nbl_lru.
> If conflock allocation fails rollback can call list_del_init()
> access uninitialized fields and corrupt memory.
> 
> Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
>  fs/nfsd/nfs4state.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 369e574c5092..176ef8d24fae 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		goto out;
>  	}
>  
> +	conflock = locks_alloc_lock();
> +	if (!conflock) {
> +		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> +		status = nfserr_jukebox;
> +		goto out;
> +	}
> +
>  	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
>  	if (!nbl) {
>  		dprintk("NFSD: %s: unable to allocate block!\n", __func__);
> @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
>  	nfs4_transform_lock_offset(file_lock);
>  
> -	conflock = locks_alloc_lock();
> -	if (!conflock) {
> -		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> -		status = nfserr_jukebox;
> -		goto out;
> -	}
> -
>  	if (fl_flags & FL_SLEEP) {
>  		nbl->nbl_time = jiffies;
>  		spin_lock(&nn->blocked_locks_lock);
> @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		status = nfserrno(err);
>  		break;
>  	}
> -out:
> -	if (nbl) {
> -		/* dequeue it if we queued it before */
> -		if (fl_flags & FL_SLEEP) {
> -			spin_lock(&nn->blocked_locks_lock);
> -			list_del_init(&nbl->nbl_list);
> -			list_del_init(&nbl->nbl_lru);
> -			spin_unlock(&nn->blocked_locks_lock);
> -		}
> -		free_blocked_lock(nbl);
> +	/* dequeue it if we queued it before */
> +	if (fl_flags & FL_SLEEP) {
> +		spin_lock(&nn->blocked_locks_lock);
> +		list_del_init(&nbl->nbl_list);
> +		list_del_init(&nbl->nbl_lru);
> +		spin_unlock(&nn->blocked_locks_lock);
>  	}
> +	free_blocked_lock(nbl);
> +out:
>  	if (nf)
>  		nfsd_file_put(nf);
>  	if (lock_stp) {

Good catch! Is there any reason not to just fix this by initializing the
list_heads in find_or_allocate_block? That seems like it'd be a simpler
fix.
Chuck Lever March 23, 2020, 1:50 p.m. UTC | #2
> On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
> 
> New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
> does not initialised nbl_list and nbl_lru.
> If conflock allocation fails rollback can call list_del_init()
> access uninitialized fields and corrupt memory.
> 
> Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> ---
> fs/nfsd/nfs4state.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 369e574c5092..176ef8d24fae 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		goto out;
> 	}
> 
> +	conflock = locks_alloc_lock();
> +	if (!conflock) {
> +		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> +		status = nfserr_jukebox;
> +		goto out;
> +	}

Nit: What do people think about removing this dprintk() as part of the fix?


> 	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
> 	if (!nbl) {
> 		dprintk("NFSD: %s: unable to allocate block!\n", __func__);
> @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> 	nfs4_transform_lock_offset(file_lock);
> 
> -	conflock = locks_alloc_lock();
> -	if (!conflock) {
> -		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> -		status = nfserr_jukebox;
> -		goto out;
> -	}
> -
> 	if (fl_flags & FL_SLEEP) {
> 		nbl->nbl_time = jiffies;
> 		spin_lock(&nn->blocked_locks_lock);
> @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		status = nfserrno(err);
> 		break;
> 	}
> -out:
> -	if (nbl) {
> -		/* dequeue it if we queued it before */
> -		if (fl_flags & FL_SLEEP) {
> -			spin_lock(&nn->blocked_locks_lock);
> -			list_del_init(&nbl->nbl_list);
> -			list_del_init(&nbl->nbl_lru);
> -			spin_unlock(&nn->blocked_locks_lock);
> -		}
> -		free_blocked_lock(nbl);
> +	/* dequeue it if we queued it before */
> +	if (fl_flags & FL_SLEEP) {
> +		spin_lock(&nn->blocked_locks_lock);
> +		list_del_init(&nbl->nbl_list);
> +		list_del_init(&nbl->nbl_lru);
> +		spin_unlock(&nn->blocked_locks_lock);
> 	}
> +	free_blocked_lock(nbl);
> +out:
> 	if (nf)
> 		nfsd_file_put(nf);
> 	if (lock_stp) {
> -- 
> 2.17.1
> 

--
Chuck Lever
J. Bruce Fields March 23, 2020, 3:09 p.m. UTC | #3
On Mon, Mar 23, 2020 at 09:50:34AM -0400, Chuck Lever wrote:
> 
> 
> > On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
> > 
> > New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
> > does not initialised nbl_list and nbl_lru.
> > If conflock allocation fails rollback can call list_del_init()
> > access uninitialized fields and corrupt memory.
> > 
> > Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
> > Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> > ---
> > fs/nfsd/nfs4state.c | 32 +++++++++++++++-----------------
> > 1 file changed, 15 insertions(+), 17 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 369e574c5092..176ef8d24fae 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > 		goto out;
> > 	}
> > 
> > +	conflock = locks_alloc_lock();
> > +	if (!conflock) {
> > +		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> > +		status = nfserr_jukebox;
> > +		goto out;
> > +	}
> 
> Nit: What do people think about removing this dprintk() as part of the fix?

I don't think we want a dprintk every place we kmalloc.  All for
removing them.--b.
Vasily Averin March 23, 2020, 10:12 p.m. UTC | #4
On 3/23/20 3:18 PM, Jeff Layton wrote:
> On Mon, 2020-03-23 at 10:55 +0300, Vasily Averin wrote:
>> New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
>> does not initialised nbl_list and nbl_lru.
>> If conflock allocation fails rollback can call list_del_init()
>> access uninitialized fields and corrupt memory.
>>
>> Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
>> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
> 
> Good catch! Is there any reason not to just fix this by initializing the
> list_heads in find_or_allocate_block? That seems like it'd be a simpler
> fix.
> 

Rollback in nfsd4_lock() is not optimal, I've tried to improve it too,
However I agree such improvement is not a simplest fix
and it anyway does not make whole rollback perfect.

I think it's better to re-send small fix for the found problem,
and prepare separate patches for rollback improvements,

Thank you,
	Vasily Averin
Chuck Lever March 26, 2020, 3:55 p.m. UTC | #5
Howdy-

> On Mar 23, 2020, at 3:55 AM, Vasily Averin <vvs@virtuozzo.com> wrote:
> 
> New struct nfsd4_blocked_lock allocated in find_or_allocate_block()
> does not initialised nbl_list and nbl_lru.
> If conflock allocation fails rollback can call list_del_init()
> access uninitialized fields and corrupt memory.
> 
> Fixes: 76d348fadff5 ("nfsd: have nfsd4_lock use blocking locks for v4.1+ lock")
> Signed-off-by: Vasily Averin <vvs@virtuozzo.com>

I'm not certain where we stand with this one. Jeff, are you OK
with me taking this for v5.7, or is there additional work needed?

I can drop the dprintk when I merge it.


> ---
> fs/nfsd/nfs4state.c | 32 +++++++++++++++-----------------
> 1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 369e574c5092..176ef8d24fae 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -6524,6 +6524,13 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		goto out;
> 	}
> 
> +	conflock = locks_alloc_lock();
> +	if (!conflock) {
> +		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> +		status = nfserr_jukebox;
> +		goto out;
> +	}
> +
> 	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
> 	if (!nbl) {
> 		dprintk("NFSD: %s: unable to allocate block!\n", __func__);
> @@ -6542,13 +6549,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 	file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
> 	nfs4_transform_lock_offset(file_lock);
> 
> -	conflock = locks_alloc_lock();
> -	if (!conflock) {
> -		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
> -		status = nfserr_jukebox;
> -		goto out;
> -	}
> -
> 	if (fl_flags & FL_SLEEP) {
> 		nbl->nbl_time = jiffies;
> 		spin_lock(&nn->blocked_locks_lock);
> @@ -6581,17 +6581,15 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> 		status = nfserrno(err);
> 		break;
> 	}
> -out:
> -	if (nbl) {
> -		/* dequeue it if we queued it before */
> -		if (fl_flags & FL_SLEEP) {
> -			spin_lock(&nn->blocked_locks_lock);
> -			list_del_init(&nbl->nbl_list);
> -			list_del_init(&nbl->nbl_lru);
> -			spin_unlock(&nn->blocked_locks_lock);
> -		}
> -		free_blocked_lock(nbl);
> +	/* dequeue it if we queued it before */
> +	if (fl_flags & FL_SLEEP) {
> +		spin_lock(&nn->blocked_locks_lock);
> +		list_del_init(&nbl->nbl_list);
> +		list_del_init(&nbl->nbl_lru);
> +		spin_unlock(&nn->blocked_locks_lock);
> 	}
> +	free_blocked_lock(nbl);
> +out:
> 	if (nf)
> 		nfsd_file_put(nf);
> 	if (lock_stp) {
> -- 
> 2.17.1
> 

--
Chuck Lever

Patch
diff mbox series

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 369e574c5092..176ef8d24fae 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -6524,6 +6524,13 @@  nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
+	conflock = locks_alloc_lock();
+	if (!conflock) {
+		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
+		status = nfserr_jukebox;
+		goto out;
+	}
+
 	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
 	if (!nbl) {
 		dprintk("NFSD: %s: unable to allocate block!\n", __func__);
@@ -6542,13 +6549,6 @@  nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	file_lock->fl_end = last_byte_offset(lock->lk_offset, lock->lk_length);
 	nfs4_transform_lock_offset(file_lock);
 
-	conflock = locks_alloc_lock();
-	if (!conflock) {
-		dprintk("NFSD: %s: unable to allocate lock!\n", __func__);
-		status = nfserr_jukebox;
-		goto out;
-	}
-
 	if (fl_flags & FL_SLEEP) {
 		nbl->nbl_time = jiffies;
 		spin_lock(&nn->blocked_locks_lock);
@@ -6581,17 +6581,15 @@  nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		status = nfserrno(err);
 		break;
 	}
-out:
-	if (nbl) {
-		/* dequeue it if we queued it before */
-		if (fl_flags & FL_SLEEP) {
-			spin_lock(&nn->blocked_locks_lock);
-			list_del_init(&nbl->nbl_list);
-			list_del_init(&nbl->nbl_lru);
-			spin_unlock(&nn->blocked_locks_lock);
-		}
-		free_blocked_lock(nbl);
+	/* dequeue it if we queued it before */
+	if (fl_flags & FL_SLEEP) {
+		spin_lock(&nn->blocked_locks_lock);
+		list_del_init(&nbl->nbl_list);
+		list_del_init(&nbl->nbl_lru);
+		spin_unlock(&nn->blocked_locks_lock);
 	}
+	free_blocked_lock(nbl);
+out:
 	if (nf)
 		nfsd_file_put(nf);
 	if (lock_stp) {