upstream nfsd kernel oops in auth_rpcgss
diff mbox

Message ID 20170130212321.GF12608@parsley.fieldses.org
State New
Headers show

Commit Message

J. Bruce Fields Jan. 30, 2017, 9:23 p.m. UTC
On Mon, Jan 30, 2017 at 04:02:37PM -0500, J. Bruce Fields wrote:
> On Mon, Jan 30, 2017 at 03:15:36PM -0500, Olga Kornievskaia wrote:
> > Hi Bruce,
> > 
> > I ran into this oops in the nfsd (below) (4.10-rc3 kernel). To trigger this I had a client (unsuccessfully) try to mount the server with krb5 where the server doesn’t have the rpcsec_gss_krb5 module built. 
> 
> Whoops, thanks for catching that.
> 
> > Below code seems to fix things:
> > 
> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
> > index 886e9d38..7faffd8 100644
> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
> > @@ -1187,9 +1187,9 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
> >  		status = -EOPNOTSUPP;
> >  		/* get mech handle from OID */
> >  		gm = gss_mech_get_by_OID(&ud->mech_oid);
> > +		rsci.cred.cr_gss_mech = gm;
> >  		if (!gm)
> >  			goto out;
> > -		rsci.cred.cr_gss_mech = gm;
> >  
> >  		status = -EINVAL;
> >  		/* mech-specific data: */
> > 
> > I guess the problem is because rsci.cred gets initialed from something that’s passed in that doesn’t have it’s cr_gss_mech initialized to NULL. So when gss_mech_get_by_OID() fails and it calls rsc_free() and tries to free a bad pointer. 
> 
> Hm, yes, just above there's:
> 
> 	rsci.cred = ud->creds;
> 
> and it does make me a little nervous if ud->creds isn't completely
> initialized.
> 
> The only caller of gss_proxy_save_rsc is svcauth_gss_proxy_init:
> 
> 	status = gss_proxy_save_rsc(sn->rsc_cache, &ud, &handle);
> 
> ud there is on the stack, and initialized by:
> 
> 	memset(&ud, 0, sizeof(ud));
> 
> Hm, in gssp_accept_sec_context_upcall, there's:
> 
> 	data->creds = *(struct svc_cred *)value->data;
> 
> That looks really weird.  I'm not sure what it should be.

Does this do it?

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Olga Kornievskaia Jan. 30, 2017, 9:36 p.m. UTC | #1
On Mon, Jan 30, 2017 at 4:23 PM, J. Bruce Fields <bfields@redhat.com> wrote:
> On Mon, Jan 30, 2017 at 04:02:37PM -0500, J. Bruce Fields wrote:
>> On Mon, Jan 30, 2017 at 03:15:36PM -0500, Olga Kornievskaia wrote:
>> > Hi Bruce,
>> >
>> > I ran into this oops in the nfsd (below) (4.10-rc3 kernel). To trigger this I had a client (unsuccessfully) try to mount the server with krb5 where the server doesn’t have the rpcsec_gss_krb5 module built.
>>
>> Whoops, thanks for catching that.
>>
>> > Below code seems to fix things:
>> >
>> > diff --git a/net/sunrpc/auth_gss/svcauth_gss.c b/net/sunrpc/auth_gss/svcauth_gss.c
>> > index 886e9d38..7faffd8 100644
>> > --- a/net/sunrpc/auth_gss/svcauth_gss.c
>> > +++ b/net/sunrpc/auth_gss/svcauth_gss.c
>> > @@ -1187,9 +1187,9 @@ static int gss_proxy_save_rsc(struct cache_detail *cd,
>> >             status = -EOPNOTSUPP;
>> >             /* get mech handle from OID */
>> >             gm = gss_mech_get_by_OID(&ud->mech_oid);
>> > +           rsci.cred.cr_gss_mech = gm;
>> >             if (!gm)
>> >                     goto out;
>> > -           rsci.cred.cr_gss_mech = gm;
>> >
>> >             status = -EINVAL;
>> >             /* mech-specific data: */
>> >
>> > I guess the problem is because rsci.cred gets initialed from something that’s passed in that doesn’t have it’s cr_gss_mech initialized to NULL. So when gss_mech_get_by_OID() fails and it calls rsc_free() and tries to free a bad pointer.
>>
>> Hm, yes, just above there's:
>>
>>       rsci.cred = ud->creds;
>>
>> and it does make me a little nervous if ud->creds isn't completely
>> initialized.
>>
>> The only caller of gss_proxy_save_rsc is svcauth_gss_proxy_init:
>>
>>       status = gss_proxy_save_rsc(sn->rsc_cache, &ud, &handle);
>>
>> ud there is on the stack, and initialized by:
>>
>>       memset(&ud, 0, sizeof(ud));
>>
>> Hm, in gssp_accept_sec_context_upcall, there's:
>>
>>       data->creds = *(struct svc_cred *)value->data;
>>
>> That looks really weird.  I'm not sure what it should be.
>
> Does this do it?
>
> --b.
>
> diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> index dc6fb79a361f..25d9a9cf7b66 100644
> --- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
> +++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
> @@ -260,7 +260,7 @@ static int gssx_dec_option_array(struct xdr_stream *xdr,
>         if (!oa->data)
>                 return -ENOMEM;
>
> -       creds = kmalloc(sizeof(struct svc_cred), GFP_KERNEL);
> +       creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
>         if (!creds) {
>                 kfree(oa->data);
>                 return -ENOMEM;

Yes, it does!

> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/net/sunrpc/auth_gss/gss_rpc_xdr.c b/net/sunrpc/auth_gss/gss_rpc_xdr.c
index dc6fb79a361f..25d9a9cf7b66 100644
--- a/net/sunrpc/auth_gss/gss_rpc_xdr.c
+++ b/net/sunrpc/auth_gss/gss_rpc_xdr.c
@@ -260,7 +260,7 @@  static int gssx_dec_option_array(struct xdr_stream *xdr,
 	if (!oa->data)
 		return -ENOMEM;
 
-	creds = kmalloc(sizeof(struct svc_cred), GFP_KERNEL);
+	creds = kzalloc(sizeof(struct svc_cred), GFP_KERNEL);
 	if (!creds) {
 		kfree(oa->data);
 		return -ENOMEM;