2015-11-20 14:56:21

by Benjamin Coddington

Subject: [PATCH] nfs4: start callback_ident at idr 1

If clp->cl_cb_ident is zero, then nfs_cb_idr_remove_locked() skips removing
it when the nfs_client is freed. A decoding or server bug can then find
and try to put that first nfs_client which would lead to a crash.

Signed-off-by: Benjamin Coddington <[email protected]>
---
fs/nfs/nfs4client.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/nfs4client.c b/fs/nfs/nfs4client.c
index 223bedd..10410e8 100644
--- a/fs/nfs/nfs4client.c
+++ b/fs/nfs/nfs4client.c
@@ -33,7 +33,7 @@ static int nfs_get_cb_ident_idr(struct nfs_client *clp, int minorversion)
return ret;
idr_preload(GFP_KERNEL);
spin_lock(&nn->nfs_client_lock);
- ret = idr_alloc(&nn->cb_ident_idr, clp, 0, 0, GFP_NOWAIT);
+ ret = idr_alloc(&nn->cb_ident_idr, clp, 1, 0, GFP_NOWAIT);
if (ret >= 0)
clp->cl_cb_ident = ret;
spin_unlock(&nn->nfs_client_lock);
--
1.7.1

2015-11-20 16:46:35

by Trond Myklebust

Subject: Re: [PATCH] nfs4: start callback_ident at idr 1

Hi Ben,

On Fri, Nov 20, 2015 at 9:56 AM, Benjamin Coddington
<[email protected]> wrote:
>
> If clp->cl_cb_ident is zero, then nfs_cb_idr_remove_locked() skips removing
> it when the nfs_client is freed. A decoding or server bug can then find
> and try to put that first nfs_client which would lead to a crash.

Thanks for fixing!
Have you ever seen such a crash in the wild? IOW: is this something we
should consider for stable?

Cheers
Trond

2015-11-20 16:56:13

by Benjamin Coddington

Subject: Re: [PATCH] nfs4: start callback_ident at idr 1

On Fri, 20 Nov 2015, Trond Myklebust wrote:

> Hi Ben,
>
> On Fri, Nov 20, 2015 at 9:56 AM, Benjamin Coddington
> <[email protected]> wrote:
> >
> > If clp->cl_cb_ident is zero, then nfs_cb_idr_remove_locked() skips removing
> > it when the nfs_client is freed. A decoding or server bug can then find
> > and try to put that first nfs_client which would lead to a crash.
>
> Thanks for fixing!
> Have you ever seen such a crash in the wild? IOW: is this something we
> should consider for stable?

Yes, there's a server that sometimes sends truncated cb_compounds, and this
bug is hit if we continue decoding 0's past the end of received data since
cb_ident ends up being 0. Both fixes I sent today are trying to handle that
particular behavior.

Ben

2015-11-20 16:57:50

by Trond Myklebust

Subject: Re: [PATCH] nfs4: start callback_ident at idr 1

On Fri, Nov 20, 2015 at 11:56 AM, Benjamin Coddington
<[email protected]> wrote:
> On Fri, 20 Nov 2015, Trond Myklebust wrote:
>
>> Hi Ben,
>>
>> On Fri, Nov 20, 2015 at 9:56 AM, Benjamin Coddington
>> <[email protected]> wrote:
>> >
>> > If clp->cl_cb_ident is zero, then nfs_cb_idr_remove_locked() skips removing
>> > it when the nfs_client is freed. A decoding or server bug can then find
>> > and try to put that first nfs_client which would lead to a crash.
>>
>> Thanks for fixing!
>> Have you ever seen such a crash in the wild? IOW: is this something we
>> should consider for stable?
>
> Yes, there's a server that sometimes sends truncated cb_compounds, and this
> bug is hit if we continue decoding 0's past the end of received data since
> cb_ident ends up being 0. Both fixes I sent today are trying to handle that
> particular behavior.
>

OK. I'll mark both for stable inclusion then.

Thanks!