Re: [PATCHv3] sunrpc/auth_gss: NULL pointer deref in gss_pipe_release()

["Alex Polvi" <polvi@google.com>]

On 8/14/06, Alex Polvi <polvi@google.com> wrote:
> On 8/9/06, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > On Wed, 2006-08-09 at 11:27 -0400, Alex Polvi wrote:
> > > On 7/31/06, Trond Myklebust <trond.myklebust@fys.uio.no> wrote:
> > > > On Mon, 2006-07-31 at 10:50 -0400, Alex Polvi wrote:
> > > > > Proposed (trivial) patch to fix a NULL pointer deref in
> > > > > gss_pipe_release(). While this does seem to fix the problem, I'm not
> > > > > entirely sure it is the correct place to handle the NULL pointer.
> > > > >
> > > > > Included below is the script I used to recreate the problem, the oops,
> > > > > and the patch.
> > > >
> > > > Sorry, but that is not the correct fix. The problem here is rather that
> > > > something is causing us to call rpc_close_pipes() on the file after the
> > > > call to gss_destroy(). That is supposed to be illegal.
> > >
> > > Since rpc_rmdir can potentially call rpc_pipe_release, here is a patch
> > > to make sure it is not called after rpcauth_destroy (which calls
> > > gss_destroy)!
> >
> > No. The current order is correct. The auth layer owns the RPCSEC_GSS
> > pipe, not the nfs_client.
> >
> > rpc_rmdir() should only be calling rpc_pipe_release() if the auth layer
> > fails to clean up after itself.
>
> Here is another fix. It is quite silly, but clnt->cl_auth is set to
> NULL in rpc_destroy_client(), then eventually referenced in
> gss_release_pipe() via rpc_rmdir(). Simply removing the clnt->cl_auth
> = NULL from clnt.c fixes the issue. I'm still trying to understand the
> subsystem, but it seems like rpc_rmdir is being correctly called to
> clean up because of the weirdness with umount -l and the nfs server
> being turned on and off. Does that seem correct? Or is this still just
> covering up some other part of the code being sloppy cleaning up?

Also, I just want to make it clear that I do not think this is the
proper fix. It is just pointing out that we intentionally set cl_auth
to NULL, then reference it.

-Alex

> Signed-off-by: Alex Polvi <polvi@google.com>
> --
> git diff net/sunrpc/clnt.c
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index d6409e7..bbe2984 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -312,10 +312,8 @@ rpc_destroy_client(struct rpc_clnt *clnt
>
>         dprintk("RPC: destroying %s client for %s\n",
>                         clnt->cl_protname, clnt->cl_server);
> -       if (clnt->cl_auth) {
> +       if (clnt->cl_auth)
>                 rpcauth_destroy(clnt->cl_auth);
> -               clnt->cl_auth = NULL;
> -       }
>         if (clnt->cl_parent != clnt) {
>                 rpc_destroy_client(clnt->cl_parent);
>                 goto out_free;
>
>
>
> > > Signed-off-by: Alex Polvi <polvi@google.com>
> > > --
> > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > index d6409e7..d2ff886 100644
> > > --- a/net/sunrpc/clnt.c
> > > +++ b/net/sunrpc/clnt.c
> > > @@ -312,16 +312,16 @@ rpc_destroy_client(struct rpc_clnt *clnt
> > >
> > >       dprintk("RPC: destroying %s client for %s\n",
> > >                       clnt->cl_protname, clnt->cl_server);
> > > -     if (clnt->cl_auth) {
> > > -             rpcauth_destroy(clnt->cl_auth);
> > > -             clnt->cl_auth = NULL;
> > > -     }
> > >       if (clnt->cl_parent != clnt) {
> > >               rpc_destroy_client(clnt->cl_parent);
> > >               goto out_free;
> > >       }
> > >       if (clnt->cl_pathname[0])
> > >               rpc_rmdir(clnt->cl_pathname);
> > > +     if (clnt->cl_auth) {
> > > +             rpcauth_destroy(clnt->cl_auth);
> > > +             clnt->cl_auth = NULL;
> > > +     }
> > >       if (clnt->cl_xprt) {
> > >               xprt_destroy(clnt->cl_xprt);
> > >               clnt->cl_xprt = NULL;
> > >
> > >
> > >
> > >
> > >
> > >
> > > > > polvi@return:~/sysops/experimental/polvi$ cat oopsmynfs.sh
> > > > > #!/bin/bash
> > > > >
> > > > > cd / # make sure we are not in the dir
> > > > >
> > > > > PROG=${0##*/}
> > > > >
> > > > > function usage {
> > > > >   cat <<EOF
> > > > > usage: $PROG /nfs/path/
> > > > >
> > > > > will oops sunrpc using the nfs host that serves /nfs/path/
> > > > > EOF
> > > > >   exit 1
> > > > > }
> > > > >
> > > > > DIR=$1
> > > > >
> > > > > [ -d "$DIR" ] || usage
> > > > >
> > > > > sudo umount $DIR 2> /dev/null # just house-keeping...
> > > > >
> > > > > sudo mount -o sec=krb5 randomfiler:/vol/to/some/share $DIR # must use krb5
> > > > >
> > > > > # with out this ls the cd below will say permission denied and not
> > > > > hang after the
> > > > > # nfs server has been turned off. It has something to do with stat64
> > > > > returning EACCES
> > > > > ls $DIR > /dev/null
> > > > >
> > > > > echo "Make the nfs server unusable by the client (turn off nfs, iptables, etc)."
> > > > > read -p "press enter when ready"
> > > > >
> > > > > # if the echo is hit, this script failed to oops sunrpc
> > > > > (cd $DIR/. ;  echo "will not cause an oops") & # this should hang
> > > > >
> > > > > sudo umount -l $DIR
> > > > >
> > > > > echo "Turn the nfs server back on and watch for the oops.  Should not
> > > > > take more then 10s"
> > > > >
> > > > >
> > > > > [  204.385339] net/sunrpc/rpc_pipe.c: rpc_lookup_parent failed to find
> > > > > path /nfs/clnt4/krb5
> > > > > [  204.385427] BUG: unable to handle kernel NULL pointer dereference
> > > > > at virtual address 0000006c
> > > > > [  204.385554]  printing eip:
> > > > > [  204.385595] c01d2322
> > > > > [  204.385678] *pde = 00000000
> > > > > [  204.385719] Oops: 0000 [#1]
> > > > > [  204.385781] SMP
> > > > > [  204.385875] Modules linked in: des binfmt_misc autofs4 video button
> > > > > battery ac nfs lockd af_packet sg sr_mod pcspkr rtc psm
> > > > >
> > > > >
> > > > >                                    ouse mousedev ide_disk ide_cd cdrom
> > > > > rpcsec_gss_krb5 auth_rpcgss sunrpc ext3 jbd mbcache thermal processor
> > > > > fan tg3 sd_mod ide_g
> > > > >
> > > > >
> > > > > eneric ata_piix libata scsi_mod generic ide_core unix
> > > > > [  204.387417] CPU:    0
> > > > > [  204.387418] EIP:    0060:[<c01d2322>]    Not tainted VLI
> > > > > [  204.387419] EFLAGS: 00010292   (2.6.18-rc2-git #1)
> > > > > [  204.387538] EIP is at _raw_spin_lock+0x12/0x170
> > > > > [  204.387578] eax: 00000068   ebx: 00000068   ecx: f7157d3c   edx: f7157d3c
> > > > > [  204.387621] esi: 00000068   edi: 00000000   ebp: f7157d00   esp: f7157cdc
> > > > > [  204.387664] ds: 007b   es: 007b   ss: 0068
> > > > > [  204.387704] Process oopsmynfs.sh (pid: 6114, ti=f7156000
> > > > > task=dffb5aa0 task.ti=f7156000)
> > > > > [  204.387748] Stack: dffb5aa0 f7157d1c c029ee7d f7157cfc f7156000
> > > > > f7156000 f73acd00 00000068
> > > > > [  204.388040]        00000000 f7157d0c c029ff7e 00000068 f7157d24
> > > > > f88d82cf f73acd00 f73acd00
> > > > > [  204.388332]        f73acecc f88e1554 f7157d50 f8cbd6a9 f73acd00
> > > > > f7288460 f73acd80 f73acd70
> > > > > [  204.388625] Call Trace:
> > > > > [  204.388868]  [<c029ff7e>] _spin_lock+0xe/0x10
> > > > > [  204.388997]  [<f88d82cf>] gss_pipe_release+0x1f/0x70 [auth_rpcgss]
> > > > > [  204.389075]  [<f8cbd6a9>] rpc_close_pipes+0xe9/0x130 [sunrpc]
> > > > > [  204.389173]  [<f8cbd91f>] rpc_depopulate+0xff/0x140 [sunrpc]
> > > > > [  204.389267]  [<f8cbda8d>] rpc_rmdir+0x6d/0xa0 [sunrpc]
> > > > > [  204.389360]  [<f8cac95e>] rpc_destroy_client+0xde/0x110 [sunrpc]
> > > > > [  204.389443]  [<f8cac8e1>] rpc_destroy_client+0x61/0x110 [sunrpc]
> > > > > [  204.389524]  [<f8cacab7>] rpc_shutdown_client+0xb7/0x120 [sunrpc]
> > > > > [  204.389605]  [<f8b2643b>] nfs_kill_super+0x3b/0x90 [nfs]
> > > > > [  204.389692]  [<c016e2c1>] deactivate_super+0x81/0xa0
> > > > > [  204.389858]  [<c0185522>] mntput_no_expire+0x52/0x90
> > > > > [  204.390039]  [<c01770da>] path_release+0x2a/0x30
> > > > > [  204.390211]  [<c01715cb>] vfs_stat_fd+0x4b/0x60
> > > > > [  204.390378]  [<c0171600>] vfs_stat+0x20/0x30
> > > > > [  204.390545]  [<c0171fc9>] sys_stat64+0x19/0x30
> > > > > [  204.390712]  [<c0102fb1>] sysenter_past_esp+0x56/0x79
> > > > > [  204.390787]  [<b7fff410>] 0xb7fff410
> > > > > [  204.390854] Code: 2b c0 89 f8 e8 00 fe ff ff e9 14 ff ff ff 8d 74
> > > > > 26 00 8d bc 27 00 00 00 00 55 89 e5 83 ec 24 89 5d f4 8b
> > > > >
> > > > >
> > > > >                                      5d 08 89 75 f8 89 7d fc <81> 7b
> > > > > 04 ad 4e ad de 75 4c 89 e0 25 00 e0 ff ff 8b 00 39 43 0c
> > > > > [  204.392662] EIP: [<c01d2322>] _raw_spin_lock+0x12/0x170 SS:ESP 0068:f7157cdc
> > > > >
> > > > > Signed-off-by: Alex Polvi <polvi@google.com>
> > > > > ---
> > > > > diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> > > > > index 4a9aa93..2db3bd1 100644
> > > > > --- a/net/sunrpc/auth_gss/auth_gss.c
> > > > > +++ b/net/sunrpc/auth_gss/auth_gss.c
> > > > > @@ -607,6 +607,9 @@ gss_pipe_release(struct inode *inode)
> > > > >        struct rpc_auth *auth;
> > > > >        struct gss_auth *gss_auth;
> > > > >
> > > > > +       if (rpci->ops == NULL)
> > > > > +               return;
> > > > > +
> > > > >        clnt = rpci->private;
> > > > >        auth = clnt->cl_auth;
> > > > >        gss_auth = container_of(auth, struct gss_auth, rpc_auth);
> > > > > -
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > > the body of a message to majordomo@vger.kernel.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > > Please read the FAQ at  http://www.tux.org/lkml/
> > > >
> > > >
> >
> >
>